Skip to content

fix(client): fix tests for createSocketUrl - #2323

Merged
alexander-akait merged 2 commits into
webpack:masterfrom
nougad:fix-createsocketurl-tests
Nov 25, 2019
Merged

fix(client): fix tests for createSocketUrl#2323
alexander-akait merged 2 commits into
webpack:masterfrom
nougad:fix-createsocketurl-tests

Conversation

@nougad

@nougad nougad commented Nov 21, 2019

Copy link
Copy Markdown
Contributor
  • 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

Motivation / Use-Case

follow up of #2303

  1. the tests are incorrect because they did not prepend the ? to the url. The parameter format should be like ?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost

  2. replacing two // with one / looks wrong an unused as mentioned in fix(client): don't override protocol for socket connection to 127.0.0.1 #2303

@codecov

codecov Bot commented Nov 21, 2019

Copy link
Copy Markdown

Codecov Report

Merging #2323 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
- Coverage   93.92%   93.83%   -0.09%     
==========================================
  Files          34       34              
  Lines        1284     1282       -2     
  Branches      366      365       -1     
==========================================
- Hits         1206     1203       -3     
- Misses         77       78       +1     
  Partials        1        1
Impacted Files Coverage Δ
client-src/default/utils/createSocketUrl.js 96.87% <100%> (-3.13%) ⬇️

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 58963e0...4df55ac. Read the comment docs.

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/cc @hiroppy @Loonride

/cc @nougad good job! big thanks

@hiroppy hiroppy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks

@hiroppy

hiroppy commented Nov 25, 2019

Copy link
Copy Markdown
Contributor

CI fails

FAIL test/e2e/ClientOptions.test.js (38.719s)
  ● Client console.log › liveReload disabled

    Uncaught, unspecified "error" event. (Error: Page crashed!)

      at Page._onTargetCrashed (node_modules/puppeteer/node6/lib/Page.js:411:10)
      at CDPSession.Page.client.on.event (node_modules/puppeteer/node6/lib/Page.js:206:56)
      at CDPSession._onMessage (node_modules/puppeteer/node6/lib/Connection.js:314:12)
      at Connection.<anonymous> (node_modules/puppeteer/node6/lib/Connection.js:173:30)
      at step (node_modules/puppeteer/node6/lib/Connection.js:130:28)
      at Promise (node_modules/puppeteer/node6/lib/Connection.js:148:16)

  ● Client console.log › liveReload enabled

    listen EADDRINUSE 0.0.0.0:8096

      at net.js:1423:9

  ● Client console.log › clientLogLevel is silent

    listen EADDRINUSE 0.0.0.0:8096

      at net.js:1423:9

@nougad

nougad commented Nov 25, 2019

Copy link
Copy Markdown
Contributor Author

@hiroppy Isn't that the test which fails every now and then on Windows? Looking at the test history a failed Error: Page crashed! is very common.

@alexander-akait
alexander-akait merged commit 27a04a6 into webpack:master Nov 25, 2019
@alexander-akait

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants