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

refactor(bazel): remove @angular/bazel protractor rule now provided by @bazel/protractor #32485

Closed

Conversation

@gregmagolan
Copy link
Contributor

commented Sep 5, 2019

Waiting on angular/components#16989.

PR Checklist
Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe: protractor_web_test_suite has been moved to @bazel/protractor (code is now in https://github.com/bazelbuild/rules_nodejs) so it can be safely removed from this repository

What is the current behavior?

@angular/bazel npm package provides npm_angular_bazel bazel repository which contains the protractor_web_test_suite rule.

What is the new behavior?

protractor_web_test_suite removed from @angular/bazel npm package (and npm_angular_bazel bazel repository). This rule is now provided by the @bazel/protractor npm package which provides the npm_bazel_protractor repository that now contains the protractor_web_test_suite

The upgrade to the latest rules_nodejs is unblocked by this as well. #32151

Does this PR introduce a breaking change?

  • Yes
  • No

BREAKING CHANGE:
Angular bazel users using protractor_web_test_suite from @angular/bazel npm package should now switch to the @bazel/protractor npm package

This should impact very few users and the user's that are impacted have a very easy upgrade path to switch to fetching the protractor_web_test_suite rule via the @bazel/protractor npm package.

@gregmagolan gregmagolan requested a review from alexeagle Sep 5, 2019
@gregmagolan gregmagolan requested a review from angular/tools-bazel as a code owner Sep 5, 2019
@googlebot googlebot added the cla: yes label Sep 5, 2019
@ngbot ngbot bot added this to the needsTriage milestone Sep 5, 2019
@gregmagolan gregmagolan force-pushed the gregmagolan:remove-angular-bazel-protractor branch from 84c11c0 to 458ea68 Sep 5, 2019
@gregmagolan gregmagolan requested review from angular/fw-core as code owners Sep 5, 2019
@gregmagolan gregmagolan force-pushed the gregmagolan:remove-angular-bazel-protractor branch from 458ea68 to 999df8d Sep 5, 2019
@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@jelbourn angular/components#16980 got merged (thanks!) but this is still failing on material-unit-test so I assume material is not being tested from head. Does this PR need to wait until angular updates to the next material release?

@JoostK

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@gregmagolan Angular's CI does indeed not run Material tests from HEAD, but rather a specific commit hash. Required changes are show-cased in #32243. Note that there's no regular update interval of the Material commit being used, so you can just update it in this PR (using a separate commit, preferably)

@gregmagolan gregmagolan requested a review from angular/dev-infra-framework as a code owner Sep 5, 2019
@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Thanks @JoostK !

@gregmagolan gregmagolan force-pushed the gregmagolan:remove-angular-bazel-protractor branch from 14a1a00 to 13148b8 Sep 5, 2019
@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@devversion @jelbourn Can you PTAL the material-unit-tests failure on CI? It looks unrelated to the protracotr remove. Something in <mat-menu> changed since the last commit-under-test?

@jelbourn

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Nothing has changed related to that, and everything builds locally when I run on master

@googlebot

This comment was marked as off-topic.

Copy link

commented Sep 6, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 6, 2019
@devversion

This comment was marked as off-topic.

Copy link
Member

commented Sep 6, 2019

@googlebot I consent.

@googlebot

This comment was marked as off-topic.

Copy link

commented Sep 6, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 6, 2019
@devversion

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@gregmagolan I pushed to this PR. There were two issues that surfaced due to the commit update:

  1. The dev-app has been restructured and some targets were accidentally built in the material-unit-tests job.
  2. Some gesture tests have been re-enabled on the components repository, but they need to be added to the blocklist here because there is a breaking change for HammerJS in angular#master.
@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Thanks @devversion !

@ngbot

This comment has been minimized.

Copy link

commented Sep 6, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

caretaker: ci/angular: size failing but looks like its happening on all PRs presently

@ngbot

This comment has been minimized.

Copy link

commented Sep 6, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

merge-assistance: global approval. the core/todo/bundle size increase seems bogus. is this a problem on master?

@gregmagolan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

is this a problem on master?

AFAIK yes. Pawel saw it here as well #32510.

gregmagolan and others added 4 commits Sep 5, 2019
…y @bazel/protractor

BREAKING CHANGE:
Angular bazel users using protractor_web_test_suite from @angular/bazel npm package should now switch to the @bazel/protractor npm package.

This should impact very few users and the user's that are impacted have a very easy upgrade path to switch to fetching the protractor_web_test_suite rule via the @bazel/protractor npm package.
Updates the SHA that will be tested against in the `material-unit-tests` job
to the latest commit in the components repository. SHA 2817c9e2faa4140342336987a692d5dd30bf24c2
is needed in order to make the `material-unit-tests` job pass after the remove
of the `protractor_web_test_suite` bazel rule from @angular/bazel. `protractor_web_test_suite` is
now provided by the @bazel/protractor npm package.
Ensures that the `material-unit-tests` job does not accidentally build
subpackages of the `src/dev-app` in the components repo. This is now
an issue because the components repository updated their dev-app
to use Bazel with more individual subpackages. These subpackages
are not excluded by the `--deleted_packages` flag.

For best practice, we should use the exclusion target pattern
instead of the undocumented `--deleted_packages` flag anyway.
Follow-up for de8ebbd. We need to
disable a few HammerJS gesture tests of the component repository by
adding them to the material-ci test blocklist.

This is now surfaces because we updated the commit of the
`material-unit-tests` job to a more recent state where we re-enabled
the HammerJS gesture tests on the components-side. The tests were
previously disabled on the components repository because the blocklist
didn't work on Angular. See:
angular/components@eaf70ca.
@gregmagolan gregmagolan force-pushed the gregmagolan:remove-angular-bazel-protractor branch from 9a8ebf7 to 8c2325a Sep 10, 2019
@matsko matsko closed this in 9448828 Sep 10, 2019
matsko added a commit that referenced this pull request Sep 10, 2019
Updates the SHA that will be tested against in the `material-unit-tests` job
to the latest commit in the components repository. SHA 2817c9e2faa4140342336987a692d5dd30bf24c2
is needed in order to make the `material-unit-tests` job pass after the remove
of the `protractor_web_test_suite` bazel rule from @angular/bazel. `protractor_web_test_suite` is
now provided by the @bazel/protractor npm package.

PR Close #32485
matsko added a commit that referenced this pull request Sep 10, 2019
…2485)

Ensures that the `material-unit-tests` job does not accidentally build
subpackages of the `src/dev-app` in the components repo. This is now
an issue because the components repository updated their dev-app
to use Bazel with more individual subpackages. These subpackages
are not excluded by the `--deleted_packages` flag.

For best practice, we should use the exclusion target pattern
instead of the undocumented `--deleted_packages` flag anyway.

PR Close #32485
matsko added a commit that referenced this pull request Sep 10, 2019
Follow-up for de8ebbd. We need to
disable a few HammerJS gesture tests of the component repository by
adding them to the material-ci test blocklist.

This is now surfaces because we updated the commit of the
`material-unit-tests` job to a more recent state where we re-enabled
the HammerJS gesture tests on the components-side. The tests were
previously disabled on the components repository because the blocklist
didn't work on Angular. See:
angular/components@eaf70ca.

PR Close #32485
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…y @bazel/protractor (angular#32485)

BREAKING CHANGE:
Angular bazel users using protractor_web_test_suite from @angular/bazel npm package should now switch to the @bazel/protractor npm package.

This should impact very few users and the user's that are impacted have a very easy upgrade path to switch to fetching the protractor_web_test_suite rule via the @bazel/protractor npm package.

PR Close angular#32485
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
Updates the SHA that will be tested against in the `material-unit-tests` job
to the latest commit in the components repository. SHA 2817c9e2faa4140342336987a692d5dd30bf24c2
is needed in order to make the `material-unit-tests` job pass after the remove
of the `protractor_web_test_suite` bazel rule from @angular/bazel. `protractor_web_test_suite` is
now provided by the @bazel/protractor npm package.

PR Close angular#32485
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…gular#32485)

Ensures that the `material-unit-tests` job does not accidentally build
subpackages of the `src/dev-app` in the components repo. This is now
an issue because the components repository updated their dev-app
to use Bazel with more individual subpackages. These subpackages
are not excluded by the `--deleted_packages` flag.

For best practice, we should use the exclusion target pattern
instead of the undocumented `--deleted_packages` flag anyway.

PR Close angular#32485
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
Follow-up for de8ebbd. We need to
disable a few HammerJS gesture tests of the component repository by
adding them to the material-ci test blocklist.

This is now surfaces because we updated the commit of the
`material-unit-tests` job to a more recent state where we re-enabled
the HammerJS gesture tests on the components-side. The tests were
previously disabled on the components repository because the blocklist
didn't work on Angular. See:
angular/components@eaf70ca.

PR Close angular#32485
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 11, 2019

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 Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.