Better test suite for binary tests #635

ematipico · 2018-10-10T08:31:27Z

What kind of change does this PR introduce?

Refactor of the test bin cases in order to run them singularly. Also, it implements snapshot tests

Did you add tests for your changes?
Well, kinda :)

If relevant, did you update the documentation?
Not yet, probably I will once everything is ready

Summary
So, taken from #621 I started the refactor of the bin tests.
As suggested, I removed from the stream of the output everything that has the Hash, timestamps and versions. This is because they change at every build/version change of webpack, causing the fail of the snapshot test.

Also, as suggested, we could change the way we pass the arguments from being an array to being an object. Feedbacks are welcom

Does this PR introduce a breaking change?

Other information
Let's have some feedback

evenstensberg · 2018-10-10T18:02:11Z

Would be nice if we can remove async timeouts and try to make jest decide it

ematipico · 2018-10-10T20:33:47Z

That's the thing. In these tests we are running the watcher and I followed the same pattern of before, where we kill the process after a while, collect the result and after that we run the assertions. Do you have a better solution?

SimenB

Awesome that you picked it up! Feel free to ping me with questions 🙂

Do you have a better solution?

Not sure if relevant or not, but in Jest we have some tests that are supposed to hang (e.g. e2e test for a server hanging forever). In that case we wait for some output to appear in stderr, and when it does, we kill the process. This allows us to wait only as long as we have to, meaning we don't waste time.

See https://github.com/facebook/jest/blob/f00fbecb4b29b1f22582255d13591344a862b88e/e2e/runJest.js#L130-L150

(it should probably implement a Promise.race to send a kill before jest times out and give a good error)

SimenB · 2018-10-14T18:03:46Z

test/testUtils.js

+	const argsWithOutput = args.concat("--output-path", outputPath);
+
+	return new Promise(resolve => {
+		// try {


You can do Promise.resolve().then(() => execa(...)).... to avoid doing new Promise. Not sure if it's better or not

SimenB · 2018-10-14T18:04:02Z

test/binCases/watch/single-config/single-config.test.js

+jest.setTimeout(10000);
+
+test("single-config", done => {
+	runWatch(__dirname, [


you can return the promise instead of using done, then jest will wait for it

ematipico · 2018-10-15T16:10:15Z

Hi @SimenB , thanks for your feedback!! Really appreciated! I tried to follow your suggestions but still we got issue inside the CI. Probably there's something that I am still missing...

So, I kill the process when there's some output (like you suggested where you wait for some kind of error). When there's output, we kill the process.

Inside jests tests, I return the promise and in theory jest should wait for it. Although on Travis it keeps failing with a timeout error, so Jest it's not waiting properly... any tips?

SimenB · 2018-10-15T17:17:41Z

Jest has a timeout of 5 seconds. If your test takes longer than that, you have to increase the timeout.

Also note that you have to remove the done callback argument when you return the promise. If your callback takes an argument, jest will wait for that argument to be invoked

evenstensberg · 2018-10-16T12:58:08Z

What Simen proposed was something I was looking for. As we don't need to wait in a fixed interval for each compilation to finish. An ETA of each compilation would be < 15s and I think 60 as a fixed limit for any compilation would be useless when we can await for output.

What would be ideal is not to spawn processes in sequence, but in parallel, as each test is isolated

ematipico · 2018-10-18T16:12:01Z

@ev1stensberg Yes it would be super awesome, but on Travis it looks like that the process takes A LOT to print out some output and because of that the test fails :(

I tried now to increase the jest timeout to 30 seconds and see how it goes.

SimenB · 2018-10-18T16:19:42Z

Oh btw, make sure to set maxWorkers on Travis of you haven't already. https://jestjs.io/docs/en/troubleshooting#tests-are-extremely-slow-on-docker-and-or-continuous-integration-ci-server

Running in band is overkill, I think there's 2 cores available on travis. You can run nproc

evenstensberg

What’s missing at this PR?

evenstensberg · 2018-10-19T18:18:13Z

package.json

-    "webpack-dev-server": "^3.1.8"
-  }
+	"name": "webpack-cli",
+	"version": "3.1.2",


Can you avoid rewriting the entire package? Hard to review

evenstensberg · 2018-10-19T18:20:12Z

test/testUtils.js

+ * @returns {Object} The webpack output
+ */
+function run(testCase, args = []) {
+	// const cwd = path.resolve(testCase);


ematipico · 2018-10-20T12:42:02Z

What's missing is finding the right way to make the watch tests working on the CI.

evenstensberg · 2018-10-30T19:14:14Z

@SimenB does this look ok?

evenstensberg · 2018-10-30T19:14:42Z

Env variables for CIs needs to be adjusted but is there anything else?

SimenB

Looks great!

You might consider running execa async instead of sync, but check the run time first.

SimenB · 2018-10-30T19:35:11Z

test/binCases/config-name/found-many/found-many.test.js

+	]);
+
+	expect(code).toBe(0);
+	expect(stdout).toEqual(expect.anything());


Unnecessary assertion

SimenB · 2018-10-30T19:36:39Z

test/binCases/config-name/found-many/found-many.test.js

+	expect(stdout).toEqual(expect.anything());
+	expect(stdout).toContain("./index2.js");
+	expect(stdout).toContain("./index3.js");
+	expect(stderr).toHaveLength(0);


toBe('')? Or toEqual

SimenB · 2018-10-30T19:42:48Z

test/binCases/configFile/plugins-precedence/__snapshots__/plugins-precedence.test.js.snap

+[0] ./index.js 265 bytes {0} [built]
+[1] ./ok.js 23 bytes {0} [built]
+
+WARNING in configuration


In Jest we have a helper called extractSummary which might be nice here. Separates summary from rest

SimenB · 2018-10-30T19:45:07Z

test/binCases/watch/info-verbosity-off/info-verbosity-off.test.js

+const { runWatch } = require("../../../testUtils");
+
+test("info-verbosity-off", () => {
+	return runWatch(__dirname, [


async-await?

We still support node 6 and we don't use Babel. I would have used it already if I could

Jest comes with babel support out of the box (that's how code coverage works), so you can just add babel-preset-env and use it in tests if you want, even if you don't want to transpile your source code. Up to you, though 🙂

Preset env is doable, but will it throw on node v6 and below?

nah, just use node: 'current' as target

I wouldn't mind to use babel for your test suite honestly 😄 It would make everything easier to write

evenstensberg

Lgtm here, removed the empty string checks @ematipico . Does this look good @SimenB ? CC @TheLarkInn can you update nproc = 4 to a env variable in Azure?

evenstensberg · 2018-11-02T22:45:53Z

Does anyone know how we can avoid the jest timeout invocation in Travis and Azure?

SimenB · 2018-11-03T00:29:29Z

Increase the timeout? CI is generally really slow unless you pay money for more cpu/ram

evenstensberg · 2018-11-03T00:42:25Z

There we go. Could you elaborate on extractSummary?

ematipico · 2018-11-03T07:32:55Z

We can have that extractSummary utility later. I'd like to get this done. We should increase the timeout to 60 seconds as before

SimenB · 2018-11-03T12:37:06Z

extractSummary in Jest is just a function that splits out the summary from everything else in stdout, allowing you to make more granular assertions about the output that's unique to a test. Mostly cosmetics, so no need to wait for it to merge this PR 🙂

evenstensberg · 2018-11-03T16:21:34Z

I think that would be nice. Let's wait for azure to update its env vars and then add a issue on it! :)

webpack-bot · 2018-11-04T00:16:57Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

SimenB · 2018-11-04T02:07:43Z

🎉

* chore(tests): PoC on new way of running integration tests * tests(refactor): started moving to a better test suite case * tests(refactoring): refactored entry point tests * tests(refactoring): refactored entry points tests * tests(refactoring): refactored configFile bins * tests(refactoring): refactored errors * tests(refactoring): ported the rest of the tests * chore(tests): remove snapshots that are dynamic * chore(tests): moved setTimout before running the tests * chore(tests): applied suggestions on async tests * chore(tests): apply timeout to wait for output from the process * chore(tests): added maxWorkers to CI * chore: revise n-workers * chore: update tests * chore: add async-await and revise babel * chore: increase timeout * chore: revise timeout * chore: more async timeouts * chore: add async timeout * chore: async

dhruvdutt

Merged as ca1e4a3.

SimenB and others added 6 commits September 30, 2018 13:33

chore(tests): PoC on new way of running integration tests

0047819

tests(refactor): started moving to a better test suite case

ce7e3be

tests(refactoring): refactored entry point tests

7d8470d

tests(refactoring): refactored entry points tests

0a8b7f0

tests(refactoring): refactored configFile bins

825c6ac

tests(refactoring): refactored errors

cd4e843

webpack-bot added the PR: unreviewed label Oct 10, 2018

ematipico mentioned this pull request Oct 10, 2018

[WIP] Better test suite for binary tests #632

Closed

tests(refactoring): ported the rest of the tests

c96f1b2

ematipico changed the title ~~[WIP] Better test suite for binary tests~~ Better test suite for binary tests Oct 10, 2018

webpack-bot added the PR: conflict label Oct 10, 2018

Merge remote-tracking branch 'upstream/master' into split-tests

ee52211

webpack-bot removed the PR: conflict label Oct 10, 2018

ematipico requested review from evenstensberg and dhruvdutt October 10, 2018 10:26

ematipico added 2 commits October 10, 2018 18:01

chore(tests): remove snapshots that are dynamic

fffef17

chore(tests): moved setTimout before running the tests

1ba0094

SimenB reviewed Oct 14, 2018

View reviewed changes

chore(tests): applied suggestions on async tests

934ea04

chore(tests): apply timeout to wait for output from the process

1673a18

chore(tests): added maxWorkers to CI

947691e

evenstensberg reviewed Oct 19, 2018

View reviewed changes

chore: revise n-workers

deba2dd

SimenB reviewed Oct 30, 2018

View reviewed changes

SimenB mentioned this pull request Oct 30, 2018

chore(tests): PoC on new way of running integration tests #621

Closed

evenstensberg added 3 commits November 2, 2018 23:14

chore: update branch

d4521eb

chore: update tests

9d31d05

chore: add async-await and revise babel

8323d9c

evenstensberg approved these changes Nov 2, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Nov 2, 2018

chore: increase timeout

232b5ec

webpack-bot added the PR: CI-not-ok label Nov 3, 2018

evenstensberg added 4 commits November 3, 2018 17:35

chore: revise timeout

2fcdd3b

chore: more async timeouts

0a399d8

chore: add async timeout

f626883

chore: async

fcea9f8

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Nov 4, 2018

ematipico merged commit 8f68555 into webpack:master Nov 4, 2018

dhruvdutt reviewed Nov 4, 2018

View reviewed changes

Better test suite for binary tests #635

Better test suite for binary tests #635

Conversation

ematipico commented Oct 10, 2018

evenstensberg commented Oct 10, 2018

ematipico commented Oct 10, 2018

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Oct 15, 2018

SimenB commented Oct 15, 2018

evenstensberg commented Oct 16, 2018

ematipico commented Oct 18, 2018 • edited

SimenB commented Oct 18, 2018 • edited

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Oct 20, 2018 • edited

evenstensberg commented Oct 30, 2018

evenstensberg commented Oct 30, 2018

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Oct 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Oct 31, 2018 • edited

Choose a reason for hiding this comment

evenstensberg left a comment

Choose a reason for hiding this comment

evenstensberg commented Nov 2, 2018

SimenB commented Nov 3, 2018

evenstensberg commented Nov 3, 2018

ematipico commented Nov 3, 2018

SimenB commented Nov 3, 2018

evenstensberg commented Nov 3, 2018

webpack-bot commented Nov 4, 2018

SimenB commented Nov 4, 2018

dhruvdutt left a comment

Choose a reason for hiding this comment

ematipico commented Oct 18, 2018 •

edited

SimenB commented Oct 18, 2018 •

edited

ematipico commented Oct 20, 2018 •

edited

ematipico Oct 30, 2018 •

edited

ematipico Oct 31, 2018 •

edited