Skip to content

test(client): add e2e reload client tests #1940

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

Merged
merged 6 commits into from
Jun 3, 2019

Conversation

knagaitsev
Copy link
Collaborator

@knagaitsev knagaitsev commented May 31, 2019

  • 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?

N/A

Motivation / Use-Case

#1933

More tests are needed for the client. These tests confirm that reloading in the browser is working.

Breaking Changes

Sometimes reload took long to occur, so I set a long wait time of 10000. I think this was fixed after I set poll: 500.

Also note that I added a temp directory in the testing directory so that my tests can write to files for reload simulation

Additional Info

Things that can still be added onto this:

  • Intercept the sockjs communications to confirm all the messages pass as expected
  • Check the console output to make sure everything is working
  • Test cases where there should be errors. For example, set hotOnly: true then do something that cannot be reloaded with hot and see the error in the console, or shut off the server early and make sure the console says WDS Disconnected

EDIT:
Another use case for this method

  • Check that the client successfully gets hot-update.json

@codecov
Copy link

codecov bot commented May 31, 2019

Codecov Report

Merging #1940 into master will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage   91.13%   91.25%   +0.11%     
==========================================
  Files          18       18              
  Lines         835      835              
  Branches      262      262              
==========================================
+ Hits          761      762       +1     
+ Misses         70       69       -1     
  Partials        4        4
Impacted Files Coverage Δ
lib/Server.js 91.77% <0%> (+0.22%) ⬆️

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 bca0341...94f7eb9. Read the comment docs.

@codecov
Copy link

codecov bot commented May 31, 2019

Codecov Report

Merging #1940 into master will increase coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage    91.8%   92.24%   +0.44%     
==========================================
  Files          23       23              
  Lines         903      903              
  Branches      283      283              
==========================================
+ Hits          829      833       +4     
+ Misses         71       67       -4     
  Partials        3        3
Impacted Files Coverage Δ
lib/Server.js 92.74% <0%> (+0.43%) ⬆️
lib/servers/SockJSServer.js 96.29% <0%> (+7.4%) ⬆️

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 44a8cde...1ce14c1. Read the comment docs.

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.

Great job!

Some notes:

  1. Let's create test_e2e directory in root
  2. Rename ClientReload.test.js to Client.test.js
  3. Maybe move other e2e test to this directory

@alexander-akait
Copy link
Member

Also need fix linting problem

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi What should I name the other e2e test if I move it into the same directory? Maybe ClientOptions.test.js since it confirms that the options work on the client?

@alexander-akait
Copy link
Member

Sounds good

@knagaitsev
Copy link
Collaborator Author

@evilebottnawi Why not test/e2e/? May be misleading if some tests are not in the test directory

@knagaitsev
Copy link
Collaborator Author

I made the test/e2e change, I can switch it if you disagree.

@alexander-akait
Copy link
Member

👍 We can refactor tests in any time, just want keep e2e test separate from unit tests, same for integration tests we should do, but not in this PR, let's do it future

.gitignore Outdated
@@ -12,3 +12,5 @@ node_modules
.vscode

.eslintcache

test/temp/*.css
Copy link
Member

Choose a reason for hiding this comment

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

Did you use editorconfig?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What does that mean? I'm not sure what editorconfig does. Is there something wrong with doing this?

Copy link
Member

@hiroppy hiroppy Jun 1, 2019

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see. I saw that too but overlooked it earlier. I think the file just needs another line at the end.

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.

/cc @hiroppy

@alexander-akait
Copy link
Member

What is wrong with azure 😕 :

@knagaitsev knagaitsev closed this Jun 1, 2019
@knagaitsev knagaitsev reopened this Jun 1, 2019
@alexander-akait alexander-akait merged commit e6d48a3 into webpack:master Jun 3, 2019
@alexander-akait
Copy link
Member

Good job!

@knagaitsev knagaitsev added the gsoc Google Summer of Code label 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.

4 participants