Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(e2e): More e2e test improvements #2163

Merged
merged 6 commits into from Aug 8, 2019

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

  • There was some problematic logic in ClientOptions.test.js
  • Client.test.js and Progress.test.js could potentially write to the same main.css, so I split their tests to use two different configs

Breaking Changes

None

Additional Info

@codecov
Copy link

codecov bot commented Jul 30, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@bbe410e). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2163   +/-   ##
=========================================
  Coverage          ?   93.85%           
=========================================
  Files             ?       33           
  Lines             ?     1270           
  Branches          ?      366           
=========================================
  Hits              ?     1192           
  Misses            ?       71           
  Partials          ?        7
Impacted Files Coverage Δ
client-src/clients/WebsocketClient.js 57.89% <50%> (ø)
client-src/clients/SockJSClient.js 61.53% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbe410e...0de0d86. Read the comment docs.

@knagaitsev
Copy link
Collaborator Author

knagaitsev commented Jul 31, 2019

/cc @evilebottnawi I don't think just adding an empty client.onerror handler is a breaking change, right? Or is it preventing something from being thrown that would normally get thrown?

Edit: I think it should not effect anything to have an empty handler https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/onerror

I got the idea that this might solve the problem from: websockets/ws#246, websockets/ws#656, but not entirely sure why it would change anything

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, it is interesting questions, in theory will be better logging error

@knagaitsev
Copy link
Collaborator Author

Page crashes still happening. I've thought about making some mechanism to retry the test if a page crashes, but that might just be sweeping an issue under the rug.

Maybe we should just wait until there is further progress on this puppeteer/puppeteer#1454

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need use logger

@@ -11,7 +11,9 @@ module.exports = class SockJSClient extends BaseClient {
super();
this.sock = new SockJS(url);

this.sock.onerror = () => {};
this.sock.onerror = (err) => {
console.error(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use logger, instead console

Copy link
Collaborator Author

@knagaitsev knagaitsev Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evilebottnawi This is another case where the path from SockJSClient to utils/log is not the same in the client-src directory compared to the client directory. I think we should wait and add this after reorganizing client directories on next, rather than do more of this redirection:

new webpack.NormalModuleReplacementPlugin(
/^\.\/clients\/SockJSClient$/,
(resource) => {
if (resource.context.startsWith(process.cwd())) {
resource.request = resource.request.replace(
/^\.\/clients\/SockJSClient$/,
'../clients/SockJSClient'
);
}
}
),

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me remove the console.error for now, since it did not exist before and we won't introduce anything new in console output.

@alexander-akait
Copy link
Member

/cc @hiroppy

@knagaitsev knagaitsev mentioned this pull request Aug 6, 2019
6 tasks
@alexander-akait alexander-akait merged commit 460f15a into webpack:master Aug 8, 2019
@knagaitsev knagaitsev added gsoc Google Summer of Code type: test labels Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code type: test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants