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

ci: do not run benchmark measurements in circleci #34753

Open
wants to merge 3 commits into
base: master
from

Conversation

@devversion
Copy link
Member

devversion commented Jan 13, 2020

Currently we run all benchmark perf tests in CircleCI. Since we do not
collect any results, we unnecessarily waste CI/RBE resources. Instead,
we should just not run benchmark perf tests in CI, but still run the
functionality e2e tests which ensure that benchmarks are not broken.

We can do this by splitting the perf and e2e tests into separate
files/targets.

@googlebot googlebot added the cla: yes label Jan 13, 2020
@devversion devversion force-pushed the devversion:build/split-benchmark-and-e2e-tests branch 3 times, most recently from a76e07c to 45060b3 Jan 13, 2020
@devversion devversion marked this pull request as ready for review Jan 13, 2020
@devversion devversion requested review from IgorMinar and angular/dev-infra-framework as code owners Jan 13, 2020
@devversion devversion force-pushed the devversion:build/split-benchmark-and-e2e-tests branch from 45060b3 to 0cbc083 Jan 13, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 13, 2020
Copy link
Member

IgorMinar left a comment

thanks @devversion! I found a few things that I think we could improve, please take a look at the individual comments.

modules/e2e_util/e2e_util.ts Outdated Show resolved Hide resolved
modules/benchmarks/src/largeform/BUILD.bazel Outdated Show resolved Hide resolved
modules/benchmarks/e2e_test.bzl Outdated Show resolved Hide resolved
devversion added 2 commits Jan 13, 2020
Currently we run all benchmark perf tests in CircleCI. Since we do not
collect any results, we unnecessarily waste CI/RBE resources. Instead,
we should just not run benchmark perf tests in CI, but still run the
functionality e2e tests which ensure that benchmarks are not broken.

We can do this by splitting the perf and e2e tests into separate
files/targets.
Currently, based on the file names it's not quite clear whether
a given `.spec.ts` file runs benchmark perf or benchmark e2e
functionality tests. To disambiguate these, we use new file
suffixs. i.e. `e2e-spec.ts` and `perf-spec.ts`.
@devversion devversion force-pushed the devversion:build/split-benchmark-and-e2e-tests branch 2 times, most recently from bb11dee to d838cac Jan 16, 2020
Previously, when the benchmark tests ran outside of Bazel, developers
had the posibility to control how the tests run through command line
options. e.g. `--dryrun`. This no longer works reliable in Bazel where
command line arguments are not passed to the text executable.

To make the global options still usable (as they could be still useful
in some cases), we just pass them through the Bazel `--test_env`. This
reduces the code we need to read the command line, but still preserves
the flexibility in a Bazel idiomatic way.
@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Jan 16, 2020

@IgorMinar Thanks for reviewing! I addressed your feedback.

@devversion devversion force-pushed the devversion:build/split-benchmark-and-e2e-tests branch from d838cac to 3a6db85 Jan 16, 2020
@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Jan 17, 2020

thank you!

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Jan 18, 2020

this PR fails in g3 in a catastrophic way with build errors due to internal latency lab tests. we'll need to investigate this and fix it next week.

@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Jan 18, 2020

I don't have enough insight, but I'd guess it's because we either changed the file names of the spec files, or since we updated the test library target names (depending on how it is set up in g3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.