Skip to content

ci: add windows buildkite #13349

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

Merged
merged 3 commits into from
Jan 14, 2019
Merged

ci: add windows buildkite #13349

merged 3 commits into from
Jan 14, 2019

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Jan 2, 2019

Related to angular/angular#27205.

@filipesilva filipesilva force-pushed the buildkite branch 15 times, most recently from 6aab8cb to 90918a6 Compare January 4, 2019 15:27
@filipesilva filipesilva requested a review from clydin January 4, 2019 16:25
@filipesilva filipesilva added target: major This PR is targeted for the next major release and removed state: WIP labels Jan 4, 2019
Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

Nice job putting this together
This looks like it takes roughly 54 minutes (about the same as AppVeyor which is an excellent start). CircleCI runs the full E2E test suite in ~50 minutes. Is there anyway to improve the performance here?

`'browserName': 'chrome'`,
`'browserName': 'chrome',
chromeOptions: {
args: ['--headless', '--no-sandbox', '--disable-gpu'],
Copy link
Member

Choose a reason for hiding this comment

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

is --disable-gpu still needed? This was originally due to a bug with windows support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might not be, no. Will remove and see if it's ok.

@@ -35,8 +35,7 @@
"validate-commits": "./bin/devkit-admin validate-commits",
"prepush": "node ./bin/devkit-admin hooks/pre-push",
"preinstall": "node ./tools/yarn/check-yarn.js",
"webdriver-update-appveyor": "webdriver-manager update --standalone false --gecko false --versions.chrome 2.37",
"webdriver-update-circleci": "webdriver-manager update --standalone false --gecko false --versions.chrome $CHROMEDRIVER_VERSION_ARG "
"webdriver-update": "webdriver-manager update --standalone false --gecko false --versions.chrome 2.45"
Copy link
Member

Choose a reason for hiding this comment

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

Hard coding the version in multiple places should be avoided if possible. Would it be possible to centralize the chrome and driver version information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm no ergonomic solution comes to mind. Storing it on a env var would need the var to be set, and a cross platform solution.

Storing it on a node module would require a roundabout way of calling webdriver-update from the node bin.

Right now it's in 2 places, which is better than before, but still would be better to be in a single place. Although to be fair it's on two places because each of them happen to install a certain version of puppeteer, and these two things happen to match (e.g. one can imagine one of these places using a different puppeteer/webdriver combo).

Did you have an approach in mind?

@filipesilva
Copy link
Contributor Author

@clydin performance wise, caching and sharding would both help right now. Caching would speed up the initial setup. Sharding would make each individual shard take less time to complete.

I have explored caching in angular/angular#27990, but haven't explored sharding. I expect we'd do something similar as today in CircleCI.

Long term further Bazel integration should make sharding moot though. Under an ideal Bazel setup the remote cache/execution would manage cache. Initial cache for node_modules and Bazel artifacts would still be of help.

@filipesilva filipesilva force-pushed the buildkite branch 2 times, most recently from 233d71e to 00b1a45 Compare January 9, 2019 14:32
@clydin
Copy link
Member

clydin commented Jan 9, 2019

Something to note though is that circleCI (on Linux) takes a total of ~48 minutes to run the full suite of 187 tests and that is with a full rebuild and setup of the E2E infrastructure four times. Whereas this and appveyor are only running 87 tests.
Is this just a windows file system access performance issue (or other windows specific concern)?

@filipesilva
Copy link
Contributor Author

I'm not aware of the specific cause of windows taking longer overall for our e2e suite. It's worth looking into further. FS is much slower on windows, yes, but that can't be the full story.

@filipesilva
Copy link
Contributor Author

We'll also need to ensure there's a hard timeout in the future. In https://buildkite.com/angular/cli/builds/26#d9a71d0c-0863-4bb5-8ab2-ffb07d89a1a6 npm install was seen taken almost two days before I manually cancelled it.

@kyliau
Copy link
Contributor

kyliau commented Jan 14, 2019

Filipe on Slack: The snapshot_publish circleci check shouldn't be running, but I think it runs because I made the branch directly on angular-cli instead of on a fork.

Merging this despite ci/circleci: snapshot_publish failing.

@kyliau kyliau merged commit 3a9230f into master Jan 14, 2019
@kyliau kyliau deleted the buildkite branch January 14, 2019 20:57
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants