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

test(ivy): test against recent Material commit #31569

Closed
wants to merge 5 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Jul 15, 2019

Updating material-unit-tests to run against master with Bazel

@JoostK JoostK added state: WIP refactoring Issue that involves refactoring or code-cleanup comp: ivy labels Jul 15, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 15, 2019
@JoostK JoostK force-pushed the update-material-unit-tests branch 2 times, most recently from 09eaa1f to c06f68b Compare July 16, 2019 16:42
.circleci/config.yml Outdated Show resolved Hide resolved
@JoostK JoostK marked this pull request as ready for review July 16, 2019 17:08
@JoostK JoostK requested a review from a team as a code owner July 16, 2019 17:08
@JoostK JoostK changed the title test(ivy): test against Material master branch test(ivy): test against stable Material tag Jul 16, 2019
@JoostK JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer PR target: TBD and removed state: WIP labels Jul 16, 2019
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM.

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
scripts/ci/run_angular_material_unit_tests.sh Outdated Show resolved Hide resolved
@devversion devversion added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 16, 2019
@JoostK JoostK requested a review from gkalpak July 16, 2019 20:28
@JoostK
Copy link
Member Author

JoostK commented Jul 16, 2019

I notice now that all tests are run in both Chromium and Firefox, compared to only Chromium when we were using gulp. The total job duration has increased from ~3:30 to 7:30, which is quite a severe regression. @devversion Can we disable the Firefox targets using a filter? That should hopefully help to reduce the execution time.

@josephperrott
Copy link
Member

@JoostK We have a tag that you can target:

Example: https://github.com/angular/components/blob/master/package.json#L21 shows only targeting chromium-local

bazel test //src/... --test_tag_filters=-e2e,-browser:firefox-local --build_tag_filters=-browser:firefox-local

@JoostK JoostK force-pushed the update-material-unit-tests branch from 6e4edef to 79a965a Compare July 16, 2019 20:59
@JoostK JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 16, 2019
@JoostK JoostK force-pushed the update-material-unit-tests branch from 79a965a to ec7a1a5 Compare July 18, 2019 16:36
@googlebot
Copy link

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.

@devversion devversion force-pushed the update-material-unit-tests branch 2 times, most recently from 35e036c to bc9413f Compare July 23, 2019 15:31
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 24, 2019
@kara kara requested a review from gkalpak July 24, 2019 22:38
.circleci/config.yml Outdated Show resolved Hide resolved
scripts/ci/update-deps-to-dist-packages.js Outdated Show resolved Hide resolved
scripts/ci/update-deps-to-dist-packages.js Outdated Show resolved Hide resolved
scripts/ci/update-deps-to-dist-packages.js Outdated Show resolved Hide resolved
scripts/ci/update-deps-to-dist-packages.js Outdated Show resolved Hide resolved
scripts/ci/update-deps-to-dist-packages.js Show resolved Hide resolved
@JoostK JoostK force-pushed the update-material-unit-tests branch from 02bcd6c to 710d971 Compare July 25, 2019 16:51
JoostK and others added 5 commits July 25, 2019 19:11
Previously, the ivy-2019 branch of the Material (aka components) repo was
used, which contains some changes that were necessary to work with Ivy.
These changes are not longer necessary, as Material's master branch is
fully working with Ivy today. To be up-to-date with recent Material
development and its support for more recent dependencies, e.g. TypeScript,
it is desirable for us to be on a newer version of Material.

This commit moves the Material tests away from the ivy-2019 branch, to a
recent commit on master. We are not targeting the master branch itself,
as that would introduce a moving target into Angular's CI checks, which
is undesirable.

Lastly, the usage of gulp to run Material's tests is changed into using
Bazel, as Material itself is now also built with Bazel.
No longer locks the Material unit tests job to a specific branch, but rather allows
locking to a specific commit from a given branch. This allows us to use the
"master" branch from the `components` repository.
@JoostK JoostK force-pushed the update-material-unit-tests branch from 710d971 to 3fea0b1 Compare July 25, 2019 17:12
@alxhub alxhub added target: major This PR is targeted for the next major release and removed PR target: TBD action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 25, 2019
@JoostK
Copy link
Member Author

JoostK commented Jul 25, 2019

We typically target master & patch for build related PRs, however the Material tests might not run correctly on the patch version (which is lacking Ivy fixes). So regarding the target, I propose this is targeting only master and I will open another PR targeting patch to see if it's green and can be merged into patch.

@kara kara unassigned JoostK Jul 25, 2019
@kara kara requested a review from gkalpak July 25, 2019 17:50
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

🎉

@kara kara added the action: merge The PR is ready for merge by the caretaker label Jul 25, 2019
@mhevery mhevery closed this in 7e46a6d Jul 25, 2019
mhevery pushed a commit that referenced this pull request Jul 25, 2019
No longer locks the Material unit tests job to a specific branch, but rather allows
locking to a specific commit from a given branch. This allows us to use the
"master" branch from the `components` repository.

PR Close #31569
JoostK added a commit to JoostK/angular that referenced this pull request Jul 25, 2019
…ar#31569)

Previously, the ivy-2019 branch of the Material (aka components) repo was
used, which contains some changes that were necessary to work with Ivy.
These changes are not longer necessary, as Material's master branch is
fully working with Ivy today. To be up-to-date with recent Material
development and its support for more recent dependencies, e.g. TypeScript,
it is desirable for us to be on a newer version of Material.

This commit moves the Material tests away from the ivy-2019 branch, to a
recent commit on master. We are not targeting the master branch itself,
as that would introduce a moving target into Angular's CI checks, which
is undesirable.

Lastly, the usage of gulp to run Material's tests is changed into using
Bazel, as Material itself is now also built with Bazel.

PR Close angular#31569
JoostK pushed a commit to JoostK/angular that referenced this pull request Jul 25, 2019
No longer locks the Material unit tests job to a specific branch, but rather allows
locking to a specific commit from a given branch. This allows us to use the
"master" branch from the `components` repository.

PR Close angular#31569
mhevery pushed a commit that referenced this pull request Jul 25, 2019
… (#31847)

Previously, the ivy-2019 branch of the Material (aka components) repo was
used, which contains some changes that were necessary to work with Ivy.
These changes are not longer necessary, as Material's master branch is
fully working with Ivy today. To be up-to-date with recent Material
development and its support for more recent dependencies, e.g. TypeScript,
it is desirable for us to be on a newer version of Material.

This commit moves the Material tests away from the ivy-2019 branch, to a
recent commit on master. We are not targeting the master branch itself,
as that would introduce a moving target into Angular's CI checks, which
is undesirable.

Lastly, the usage of gulp to run Material's tests is changed into using
Bazel, as Material itself is now also built with Bazel.

PR Close #31569

PR Close #31847
mhevery pushed a commit that referenced this pull request Jul 25, 2019
No longer locks the Material unit tests job to a specific branch, but rather allows
locking to a specific commit from a given branch. This allows us to use the
"master" branch from the `components` repository.

PR Close #31569

PR Close #31847
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…ar#31569)

Previously, the ivy-2019 branch of the Material (aka components) repo was
used, which contains some changes that were necessary to work with Ivy.
These changes are not longer necessary, as Material's master branch is
fully working with Ivy today. To be up-to-date with recent Material
development and its support for more recent dependencies, e.g. TypeScript,
it is desirable for us to be on a newer version of Material.

This commit moves the Material tests away from the ivy-2019 branch, to a
recent commit on master. We are not targeting the master branch itself,
as that would introduce a moving target into Angular's CI checks, which
is undesirable.

Lastly, the usage of gulp to run Material's tests is changed into using
Bazel, as Material itself is now also built with Bazel.

PR Close angular#31569
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
No longer locks the Material unit tests job to a specific branch, but rather allows
locking to a specific commit from a given branch. This allows us to use the
"master" branch from the `components` repository.

PR Close angular#31569
@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 15, 2019
@JoostK JoostK deleted the update-material-unit-tests branch September 15, 2019 13:02
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 cla: yes refactoring Issue that involves refactoring or code-cleanup 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

7 participants