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(server): increase port mapping base number #2148

Merged
merged 1 commit into from
Jul 27, 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?

N/A

Motivation / Use-Case

As mentioned in #2143, if we need to test CLI default port selection, which will use ports 8080, 8081, 8082... we should make sure the hard port mapping does not use these ports near port 8080. We just need to provide enough room for this default port assignment so that there will never be collisions.

Breaking Changes

None

Additional Info

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #2148 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2148   +/-   ##
=======================================
  Coverage   92.56%   92.56%           
=======================================
  Files          33       33           
  Lines        1265     1265           
  Branches      361      361           
=======================================
  Hits         1171     1171           
  Misses         87       87           
  Partials        7        7

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 b79f523...5fa0cf0. Read the comment docs.

@@ -46,7 +46,7 @@ const portsList = {
Iframe: 1,
};

let startPort = 8079;
let startPort = 8089;
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this? I thought it was only necessary to change the number of cli from 2 to 10.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hiroppy There are other tests that use the default ports near 8080, such as: https://github.com/webpack/webpack-dev-server/blob/master/test/server/utils/findPort.test.js. So I think we should provide some extra room to make sure those ports are available.

@hiroppy hiroppy merged commit 72aee7e into webpack:master Jul 27, 2019
knagaitsev added a commit to knagaitsev/webpack-dev-server that referenced this pull request Jul 31, 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.

4 participants