test(e2e): improve puppeteer argument flags #2127

knagaitsev · 2019-07-16T22:01:00Z

For Bugs and Features; did you add new tests?

N/A

Motivation / Use-Case

Use the recommended args for puppeteer from https://github.com/alixaxel/chrome-aws-lambda/blob/master/source/index.js

Hopefully this could make e2e tests more stable.

Breaking Changes

None

Additional Info

codecov · 2019-07-16T22:05:27Z

Codecov Report

Merging #2127 into master will decrease coverage by 0.13%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #2127      +/-   ##
==========================================
- Coverage   92.67%   92.53%   -0.14%     
==========================================
  Files          32       32              
  Lines        1256     1260       +4     
  Branches      360      360              
==========================================
+ Hits         1164     1166       +2     
- Misses         85       87       +2     
  Partials        7        7

Impacted Files	Coverage Δ
lib/servers/SockJSServer.js	`93.75% <50%> (-2.92%)`	⬇️
lib/servers/WebsocketServer.js	`89.47% <50%> (-4.65%)`	⬇️

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 75718b7...7f6712f. Read the comment docs.

hiroppy · 2019-07-17T06:49:00Z

fmm.. CI fails. 😞 sometimes CI is stopped by Azure because of timeout.

alexander-akait · 2019-07-17T09:52:42Z

@Loonride maybe we can add timeout for puppeteer to avoid 60 minutes

knagaitsev · 2019-07-18T05:01:22Z

/cc @evilebottnawi @hiroppy Got it working! Turns out it was some timing issues.

hiroppy · 2019-07-18T06:38:29Z

👍 This test seems to be a flaky test.

FAIL test/e2e/Progress.test.js (12.03s)
  ● client progress › using hot › on browser client › should console.log progress

    Error [ERR_UNHANDLED_ERROR]: Unhandled error. (Error: Page crashed!

      at Page._onTargetCrashed (node_modules/puppeteer/lib/Page.js:230:24)
      at CDPSession.Page.client.on.event (node_modules/puppeteer/lib/Page.js:175:56)
      at CDPSession._onMessage (node_modules/puppeteer/lib/Connection.js:252:12)
      at Connection._onMessage (node_modules/puppeteer/lib/Connection.js:143:28)
      at WebSocketTransport._ws.addEventListener.event (node_modules/puppeteer/lib/WebSocketTransport.js:47:42)
      at WebSocket.onMessage (node_modules/ws/lib/event-target.js:127:16)
      at Receiver.receiverOnMessage (node_modules/ws/lib/websocket.js:746:20)
      at Receiver.dataMessage (node_modules/ws/lib/receiver.js:428:14)
      at Receiver.getData (node_modules/ws/lib/receiver.js:360:17)
      at Receiver.startLoop (node_modules/ws/lib/receiver.js:152:22)
      at Receiver._write (node_modules/ws/lib/receiver.js:83:10)
      at Socket.socketOnData (node_modules/ws/lib/websocket.js:819:35)
      at TCP.onStreamRead [as onread] (internal/stream_base_commons.js:94:17))
      at Page._onTargetCrashed (node_modules/puppeteer/lib/Page.js:230:10)
      at CDPSession.Page.client.on.event (node_modules/puppeteer/lib/Page.js:175:56)
      at CDPSession._onMessage (node_modules/puppeteer/lib/Connection.js:252:12)
      at Connection._onMessage (node_modules/puppeteer/lib/Connection.js:143:28)
      at WebSocketTransport._ws.addEventListener.event (node_modules/puppeteer/lib/WebSocketTransport.js:47:42)
      at WebSocket.onMessage (node_modules/ws/lib/event-target.js:127:16)
      at Receiver.receiverOnMessage (node_modules/ws/lib/websocket.js:746:20)

knagaitsev · 2019-07-19T02:44:31Z

/cc @evilebottnawi @hiroppy Still possibly not perfect, but appears more reliable. Here are my changes:

use testSequencer.js to space out tests such that e2e tests are evenly spread out. I suspect that running many e2e tests concurrently can lead to problems
added more timing wait events - this will increase test time a bit but I think it is worth it
use done() callback instead of returning a Promise in e2e tests. Not sure if this makes any difference, but the e2e tests that returned promises were being flaky.
Always do expect(...) testing right before the done() call. I saw strange behavior once before when there was a delay between expect testing and the final callback.

alexander-akait

Good job, some notes

alexander-akait · 2019-07-19T10:41:19Z

lib/servers/SockJSServer.js

+    // prevent cases where the server is trying to send data while connection is closing
+    if (connection.readyState === 1) {
+      connection.write(message);
+    }


hm, let's rewrite

if (connection.readyState !== 1) { return; } connection.write(message);

Hope it is not break code 😄

alexander-akait · 2019-07-19T10:42:10Z

lib/servers/WebsocketServer.js

+    // prevent cases where the server is trying to send data while connection is closing
+    if (connection.readyState === 1) {
+      connection.send(message);
+    }


alexander-akait · 2019-07-19T10:43:41Z

test/e2e/ClientMode.test.js

@@ -54,17 +54,29 @@ describe('clientMode', () => {
              res.push(_text);
            });

-            setTimeout(() => {
+            page.waitFor(3000).then(() => {


Magic number, let's create file test/server/helpers/server-constans.js and move this values in variable with name

alexander-akait · 2019-07-19T10:44:22Z

test/e2e/__snapshots__/ClientOptions.test.js.snap

@@ -31,5 +31,6 @@ Array [
 exports[`Client console.log liveReload enabled 1`] = `
 Array [
  "Hey.",
+  "[WDS] Live Reloading enabled.",


why it is appears here?

@evilebottnawi Previously, inline was disabled for this test, so the live reload message was not printed. Look at the change in test-server.js. I made it so that options.inline is not set automatically to false when options.liveReload exists. This test makes more sense if inline is enabled.

alexander-akait · 2019-07-19T10:44:59Z

test/helpers/run-browser.js

+          '--prerender-from-omnibox=disabled',
+          '--use-gl=swiftshader',
+          '--use-mock-keychain',
+        ],


Let's move this in variable and put above in file or maybe in other file

alexander-akait · 2019-07-19T10:46:02Z

test/testSequencer.js

+  }
+}
+
+module.exports = CustomSequencer;


Can you add docs in above function and describe what it is do and why we need this, it can be misleading for other developers without description

knagaitsev · 2019-07-19T17:20:06Z

@evilebottnawi Should I add a test for the readyState !== 1 feature?

alexander-akait · 2019-07-19T17:20:52Z

test/helpers/server-constants.js

@@ -0,0 +1,52 @@
+'use strict';


Let's name file puppeteer-constaints.js

alexander-akait

One note, good work!

knagaitsev · 2019-07-19T17:54:31Z

Oddly on the Windows node-8 test failure, the tests did not run in the specified order

alexander-akait · 2019-07-19T17:57:25Z

@Loonride maybe something wrong with paths?

knagaitsev · 2019-07-19T17:57:52Z

It looks like ordering isn't working on Windows CI, probably because of path yes, I will investigate

Edit: Yes, Windows test paths using backslash

test(e2e): improve puppeteer argument flags

6a14a9c

knagaitsev requested review from alexander-akait and hiroppy as code owners July 16, 2019 22:01

test(e2e): wait for page load before checking console output

17e131d

knagaitsev added 2 commits July 17, 2019 20:58

test(e2e): improve timing on client test

1d3f30f

test(e2e): remove failed connection error messages at end of client mode

0e777c1

test(e2e): move client mode test delay back down

a72785c

evilebottnawi and others added 8 commits July 18, 2019 13:01

Merge branch 'master' into e2e-test-improvements

a1686bc

test(e2e): improve timing, prevent sending at wrong readyState

14dcf18

test(e2e): update client options snapshot

40c8342

test(e2e): add pause before closing browser

14db723

test(server): moved base test port up by 10

7460fc3

test(e2e): change timing again, change watch options

b293750

test(e2e): make client test run first

e5fbb06

test(e2e): space out when e2e tests are run due to concurrency

6ef06cd

test(e2e): back to original port map, do test right before callback

ae0e06f

alexander-akait requested changes Jul 19, 2019

View reviewed changes

test(e2e): server constants, explain sequencer

af15036

alexander-akait reviewed Jul 19, 2019

View reviewed changes

test(e2e): rename server constants to puppeteer constants

8a96cfb

knagaitsev and others added 2 commits July 19, 2019 13:09

test(e2e): fix path testing in test sequencer for windows

1baded3

Merge branch 'master' into e2e-test-improvements

7f6712f

hiroppy approved these changes Jul 23, 2019

View reviewed changes

alexander-akait approved these changes Jul 23, 2019

View reviewed changes

alexander-akait merged commit 05bdb0c into webpack:master Jul 23, 2019

knagaitsev added a commit to knagaitsev/webpack-dev-server that referenced this pull request Jul 31, 2019

test(e2e): improve puppeteer argument flags (webpack#2127)

40dca93

knagaitsev added gsoc Google Summer of Code type: test labels Aug 13, 2019

Uh oh!

test(e2e): improve puppeteer argument flags #2127

test(e2e): improve puppeteer argument flags #2127

Uh oh!

Conversation

knagaitsev commented Jul 16, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

Uh oh!

codecov bot commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hiroppy commented Jul 17, 2019

Uh oh!

alexander-akait commented Jul 17, 2019

Uh oh!

knagaitsev commented Jul 18, 2019

Uh oh!

hiroppy commented Jul 18, 2019

Uh oh!

knagaitsev commented Jul 19, 2019

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Jul 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Jul 19, 2019

Uh oh!

alexander-akait commented Jul 19, 2019

Uh oh!

knagaitsev commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 16, 2019 •

edited

Loading

knagaitsev commented Jul 19, 2019 •

edited

Loading