test: Preact CLI tests #107

rkostrzewski · 2017-06-10T10:10:20Z

Basic tests for every command CLI exposes 🎉

Stuff used:

Headless chrome (which requires Chrome > 59)
Tape - Jest had problems with running Chrome, Ava had problems with spawning - tests are kinda slow without concurrency (~1m on my laptop).

Stuff changed:

Added option to preact create not to install dependencies

Refactor, testing build size

Headless chrome, all commands tested

lukeed

Chrome stuff looks really cool!

I'm also not sure that there's a need for async-test at all. The test function accepts an opts.timeout value which defaults to 500.

test('possibly long test', { timeout:30000 }, async t => {
  // ...
});

Feel free to disregard anything 😉 Thanks for taking the time to do all this!

lukeed · 2017-06-10T22:50:24Z

src/commands/create.js

+				] : [])
+			].filter(Boolean));
+		}
 		spinner.text = 'Installing dependencies';


Might want to move this all under the same if (argv.install). Plus, right now this will display "Installing deps..." when install = false.

lukeed · 2017-06-10T22:59:07Z

tests/build.test.js

+	'sw.js': { size: 3905 }
+};
+
+const expectedOutputs = prepare({


Can totally disregard this, but I typically like to move "expectants" of this size to a separate fixture file.

Yeah, you're right - just being lazy 😛 I was thinking about some snapshots but the only package i found didn't work

lukeed · 2017-06-10T23:02:53Z

tests/create.test.js

+const listOutput = async dir => await lsr(dir, ['.gitkeep', 'package.json']);
+
+test('preact create - before', async () => {
+	await setup();


I'd also prefer to move all the setup()s and clean()s into the test itself, since the presence of async will ensure that those prep/teardowns happen accordingly.

I separated them because it the 'main' test fails then they will be called for sure.

It doesn't matter for create or build that much (they could be handled inside tests and in test.onFinish hook), but stuff with chrome get's trickier as we must ensure service worker is unregistered after tests because on non-CI environment service workers could provide false positives (I've tried using profile flag to ensure clean state to no avail).

I thought that consistency between all tests would be easier to maintain & extend.

lukeed · 2017-06-10T23:12:21Z

tests/lib/output.js

+
+export const outputPath = resolve(__dirname, '../output');
+
+export const setup = async () => {


Both of these can just be:

export const name = () => helper(foo);

since they are returning Promises & will be/are awaited in the tests.

That's what you get for writing in language with native async & await for years. 😅

rkostrzewski · 2017-06-10T23:31:15Z

@lukeed I was afraid of DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.. Since tape doesn't care about returned promises async-test takes care of that.

opts.timeout looks good - I'll definitely use it!

lukeed · 2017-06-10T23:42:44Z

Where does that DeprecationWarning show up?

rkostrzewski · 2017-06-10T23:44:40Z

It comes right out of node in console when async test fails using regular tape test.

lukeed · 2017-06-10T23:47:51Z

Oh sorry, I meant a specific test as an example 😃

rkostrzewski · 2017-06-10T23:59:13Z

Then any test hopefully never 😄

CR changes

lukeed

Nice, thanks! 👍 My only suggestion now is to just write a timeout:Number directly into the async-test so that you don't have to define an options object on only some of the tests.

Plus, having the shared timeout won't affect any tests negatively, since the ones that currently don't have a timeout are expected to finish quickly.

Well done!

developit

This is basically perfect, I have no valuable comments to add lol.

developit · 2017-06-11T14:59:51Z

src/commands/create.js

 			type: 'boolean',
 			default: false
+		},
+		install: {


developit · 2017-06-11T15:00:34Z

src/commands/create.js


 		await fs.writeFile(path.resolve(target, 'package.json'), JSON.stringify(pkg, null, 2));

 		spinner.text = 'Installing dev dependencies';


super nitpick (feel free to ignore) - seems like the dev dep message and "done!" message should be inside the if here.

Oh, I misread the diff last time. Yeah, ^^ what I was trying to say last time. 😴

And I wanted to change it like that! Anyway, should be good now 😆

developit · 2017-06-11T15:20:33Z

@rkostrzewski @lukeed think we're good to merge? It'd be great to have these running on Travis. To that end, I just enabled travis for the preact-cli repo. I think updating this branch will trigger a build and we can check that everything is peachy there.

rkostrzewski · 2017-06-11T19:16:14Z

@developit hold on I'll work on setting up travis on this branch - I've got a feeling it won't be so easy (I bet timeouts will have to be increased 😝 )

@lukeed I was afraid that on CI dev server tests will run for more than a minute and timeout will get large and could negatively affect hanging tests on PRs (like waiting 15minutes to get result etc).

Try no 1

Try no 2

This reverts commit 266443b.

This reverts commit 9395884.

rkostrzewski · 2017-06-13T12:11:19Z

...many commits later....

It's green! 🎉 😀 I've restarted the build several times to make sure it is reliable.

@lukeed assuming you mean sth like test('Name', 500, async t => I'll leave it like it is - I think current approach with async test having same API as tape test is more versatile.

@developit I think we're ready to merge! 😀

BTW we can install yarn on travis after yarn PR makes it way and use it to create app - maybe the install switch could be removed

developit · 2017-06-13T13:19:58Z

That's awesome :D

# Conflicts: # .gitignore

# Conflicts: # .travis.yml

rkostrzewski · 2017-06-20T13:53:58Z

| rkostrzewski dismissed stale reviews from @developit and @lukeed via 7800035 9 days ago
😢 any of you mind doing it again? 🙏

developit

Just the one question about sleep

developit · 2017-06-23T15:07:32Z

tests/serve.test.js

+	let { listeners } = await DOMDebugger.getEventListeners({ objectId: object.objectId });
+
+	if (!listeners.some(l => l.type === 'click')) {
+		await pageIsInteractive(chrome, retryCount - 1, retryInterval);


I think this might be missing an await sleep(retryInterval)?

Preact sure is fast as it didn't ever fail on CI AFAIR. 😄 I'll add the delay with smaller interval

Done so here.

Is there any trick not to dismiss reviews when committing?

not sure, maybe it's a setting..

yup, just turned that thing off. sorry!

developit

LGTM!

lukeed · 2017-06-23T16:21:51Z

🙌 🕺

rkostrzewski added 4 commits June 7, 2017 21:44

test: Preact CLI tests [WIP]

864e55c

test: Preact CLI tests [WIP]

8d5d286

Refactor, testing build size

test: Preact CLI tests

a70e062

Headless chrome, all commands tested

Remove yarn.lock

f0b2020

rkostrzewski requested review from developit and lukeed June 10, 2017 22:37

lukeed requested changes Jun 10, 2017

View reviewed changes

lukeed reviewed Jun 10, 2017

View reviewed changes

refactor: Preact CLI tests

69829bb

CR changes

lukeed previously approved these changes Jun 11, 2017

View reviewed changes

developit previously approved these changes Jun 11, 2017

View reviewed changes

chore: Setup Travis CI

7800035

rkostrzewski dismissed stale reviews from developit and lukeed via 7800035 June 11, 2017 20:17

rkostrzewski added 9 commits June 11, 2017 22:35

fix: Setup test timeouts for travis

2197b93

Try no 1

chore: Travis setup chrome path [WIP]

32d6cfb

fix: Setup test timeouts for travis

8ae3a8d

Try no 2

chore: Debug Chrome errors on Travis

266443b

Revert "chore: Debug Chrome errors on Travis"

332f858

This reverts commit 266443b.

chore: Travis setup chrome

24840c6

chore: Travis setup chrome

0706424

chore: Travis setup chrome

b6d179a

chore: Travis setup chrome

0eed33a

rkostrzewski added 9 commits June 12, 2017 15:50

chore: Travis setup chrome

ddd4700

chore: Travis setup chrome

6a4b7e7

chore: Travis setup chrome

2546b75

refactor: Preact CLI tests

f64422b

test: Preact CLI increase build timeout

9b3bea3

test: Increase robustness of serve test

17e8aa3

test: Don't fail when page can't be loaded

9395884

Revert "test: Don't fail when page can't be loaded"

5c10410

This reverts commit 9395884.

test: Increase robustness of serve test

cbe6052

rkostrzewski added 6 commits June 15, 2017 17:12

Merge branch 'master' into feature/cli-tests

7a3f7bd

# Conflicts: # .gitignore

tests: Use chrome-launcher npm package instead of lighthouse

bf1b859

tests: Remove cert install

1f74d82

tests: New way of handling SSL certificates

ecb762e

tests: New way of handling SSL certificates

4e0ec76

Merge branch 'master' into feature/cli-tests

94038b7

# Conflicts: # .travis.yml

developit previously approved these changes Jun 23, 2017

View reviewed changes

tests: Add delay to page interactive check for serve.test.js

59e1293

rkostrzewski dismissed developit’s stale review via 59e1293 June 23, 2017 15:44

developit approved these changes Jun 23, 2017

View reviewed changes

rkostrzewski added this to the 1.2.0 milestone Jun 23, 2017

rkostrzewski merged commit bf62766 into master Jun 23, 2017

rkostrzewski deleted the feature/cli-tests branch June 23, 2017 15:58

chagretes mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities chagretes/preact-cli#29

Open


		export const outputPath = resolve(__dirname, '../output');

		export const setup = async () => {


		await fs.writeFile(path.resolve(target, 'package.json'), JSON.stringify(pkg, null, 2));

		spinner.text = 'Installing dev dependencies';

Uh oh!

test: Preact CLI tests #107

test: Preact CLI tests #107

Uh oh!

Conversation

rkostrzewski commented Jun 10, 2017

Uh oh!

lukeed 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!

rkostrzewski commented Jun 10, 2017

Uh oh!

lukeed commented Jun 10, 2017

Uh oh!

rkostrzewski commented Jun 10, 2017

Uh oh!

lukeed commented Jun 10, 2017

Uh oh!

rkostrzewski commented Jun 10, 2017

Uh oh!

lukeed left a comment

Choose a reason for hiding this comment

Uh oh!

developit 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!

developit commented Jun 11, 2017

Uh oh!

rkostrzewski commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkostrzewski commented Jun 13, 2017

Uh oh!

developit commented Jun 13, 2017

Uh oh!

rkostrzewski commented Jun 20, 2017

Uh oh!

developit 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!

rkostrzewski Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

developit left a comment

Choose a reason for hiding this comment

Uh oh!

lukeed commented Jun 23, 2017

Uh oh!

Reviewers

rkostrzewski commented Jun 11, 2017 •

edited

Loading

rkostrzewski Jun 23, 2017 •

edited

Loading