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

build(bazel): fix //modules/benchmarks/src/largetable/render3:perf benchmark in CI #26908

Closed
wants to merge 1 commit into from

Conversation

gregmagolan
Copy link
Contributor

@gregmagolan gregmagolan commented Nov 1, 2018

Underlying issue was that //modules/benchmarks/src/largetable/render3:perf relies on the locally installed version of chrome. It uses protractor_web_test and not protractor_web_test_suite. The latter provides its own version of chrome via rules_webtesting, the former does not.

To fix it, the browsers_docker_image docker image is now used for test_ivy_aot and test_ivy_jit and the benchmark is also run locally and not on RBE.

@mary-poppins
Copy link

You can preview 9e69abb at https://pr26908-9e69abb.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 124973e at https://pr26908-124973e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 599b5ef at https://pr26908-599b5ef.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 426bd23 at https://pr26908-426bd23.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 816b377 at https://pr26908-816b377.ngbuilds.io/.

@gregmagolan gregmagolan force-pushed the fix-ci-benchmark branch 2 times, most recently from 076b696 to 16da469 Compare November 2, 2018 20:43
@gregmagolan gregmagolan changed the title [WIP] build(bazel): run benchmark in ci build(bazel): fix //modules/benchmarks/src/largetable/render3:perf benchmark in CI Nov 2, 2018
@gregmagolan gregmagolan added area: build & ci Related the build and CI infrastructure of the project area: bazel Issues related to the published `@angular/bazel` build rules labels Nov 2, 2018
@ngbot ngbot bot added this to the needsTriage milestone Nov 2, 2018
@gregmagolan gregmagolan added the target: patch This PR is targeted for the next patch release label Nov 2, 2018
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Is there a reason why we can't switch to the other rule that brings in its own browsers? The fewer exceptions and customizations we make to this setup the easier it is to maintain it. But maybe there is a valid reason and in that case we should document it in the circle ci config where we pull in the custom docker image.

@mary-poppins
Copy link

You can preview 038ffe3 at https://pr26908-038ffe3.ngbuilds.io/.

@gregmagolan gregmagolan force-pushed the fix-ci-benchmark branch 4 times, most recently from c616276 to 875653d Compare November 7, 2018 22:36
@mary-poppins
Copy link

You can preview bfbe09d at https://pr26908-bfbe09d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 875653d at https://pr26908-875653d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 99732ae at https://pr26908-99732ae.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ddd35bc at https://pr26908-ddd35bc.ngbuilds.io/.

@gregmagolan
Copy link
Contributor Author

Updated to use protractor_web_test_suite so that chrome is not needed locally anymore. It needed an update to the protractor rule to merge capabilities but looks like it works like a charm.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Thanks!

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Nov 7, 2018
@IgorMinar IgorMinar removed the request for review from kara November 7, 2018 23:50
@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Nov 8, 2018
AndrewKushnir pushed a commit that referenced this pull request Nov 8, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules area: build & ci Related the build and CI infrastructure of the project cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants