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

Breaking: Spawn webserver in separate process #1758

Merged
merged 2 commits into from Jan 28, 2019
Merged

Conversation

molant
Copy link
Member

@molant molant commented Jan 24, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

This currently spawns a new server per test. I was thinking about doing something smart and have just one per test file but ava spawns every test in parallel so I'm not sure that would change anything.

Also this PR is missing:

  • Update the testing documentation on how the test server works
  • Update the create-hint package

But it will be good if you can start taking a look into it.

Fix #1694

@molant
Copy link
Member Author

molant commented Jan 24, 2019

BTW, I passed all the tests on Windows for the first time in a very long time 😊

package.json Outdated Show resolved Hide resolved
packages/connector-chrome/tests/invalid-url.ts Outdated Show resolved Hide resolved
packages/connector-chrome/tests/requestResponse.ts Outdated Show resolved Hide resolved
packages/connector-chrome/tests/requestResponse.ts Outdated Show resolved Hide resolved
packages/connector-chrome/tests/fetchContent.ts Outdated Show resolved Hide resolved
packages/connector-chrome/tests/evaluate.ts Outdated Show resolved Hide resolved
packages/connector-jsdom/tests/evaluate.ts Outdated Show resolved Hide resolved
packages/connector-jsdom/tests/fetchContent.ts Outdated Show resolved Hide resolved
@molant
Copy link
Member Author

molant commented Jan 24, 2019

@sarvaje I've done your feedback

@molant
Copy link
Member Author

molant commented Jan 24, 2019

  • Update the create-hint package ➡ Doesn't need any manual update. We mark this as a breaking change so the release script updates everything.

@alrra, did you change the script so major version bumps of devDependencies are patches/minors?

@sarvaje
Copy link
Contributor

sarvaje commented Jan 24, 2019

@molant why are you ignoring connector-chrome in so many tests?

packages/utils-create-server/README.md Outdated Show resolved Hide resolved
@molant
Copy link
Member Author

molant commented Jan 24, 2019

why are you ignoring connector-chrome in so many tests?

Windows build keep failing on Windows and I'm testing things to make it work. I was expecting them to be more reliable by spawning less chrome instances :(

@sarvaje
Copy link
Contributor

sarvaje commented Jan 25, 2019

Windows build keep failing on Windows and I'm testing things to make it work. I was expecting them to be more reliable by spawning less chrome instances :(

But it is just for testing, or is it going to be permanent?

@molant
Copy link
Member Author

molant commented Jan 25, 2019

But it is just for testing, or is it going to be permanent?

🤷‍♂️, I'm doing tests for now

@molant molant force-pushed the fix/1694 branch 4 times, most recently from 6373ae3 to de3127d Compare January 25, 2019 03:32
@molant molant mentioned this pull request Jan 25, 2019
3 tasks
@alrra
Copy link
Contributor

alrra commented Jan 25, 2019

@alrra, did you change the script so major version bumps of devDependencies are patches/minors?

Done.

@molant molant force-pushed the fix/1694 branch 4 times, most recently from b858d27 to ff9f33f Compare January 25, 2019 21:54
@molant molant changed the title Fix: Spawn webserver in separate process Breaking: Spawn webserver in separate process Jan 25, 2019
@molant
Copy link
Member Author

molant commented Jan 25, 2019

I think this is ready for merge. I've done a few changes since the last review so it will be cool if you could review it again:

  • Fix a race condition with CDP that was causing the Windows failures (that's what I believe, Windows builds are now passing on Pipelines 🎉)
  • Refactor requestResponse in jsdom to match the one in chrome (forgot about it last time, both files should look about the same now)
  • group some awaits into await Promise.all()

Also this PR should be rebased instead of squashed. I can do it later today if I get enough 👍

@molant
Copy link
Member Author

molant commented Jan 25, 2019

@antross can you please take another look? 🙏 🥺
Can't wait to get this merged

Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

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

Looks good to me with the updates assuming tests pass.

w4C7/uFcTns/5rYjv+vp+EGv5cupxJjXpd1O/buT/Tt/Ur5maySbu4vDYVgmPuXU
7avEZJTeYgih
-----END CERTIFICATE-----`
private static readonly _key: string =
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to have these thinks hardcoded and not in a files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perf, that way we avoid reading from disk and it's not like we are going to change them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok!

@molant
Copy link
Member Author

molant commented Jan 26, 2019

Looks good to me with the updates assuming tests pass.

Tests pass! mac is failing because of #1772 which happens randomly but that's unrelated to this PR.

@molant molant merged commit f6336aa into webhintio:master Jan 28, 2019
@molant molant deleted the fix/1694 branch January 28, 2019 20:00
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.

None yet

4 participants