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

Clean test_{runner,scalafmt_helper}, add diagnostic env vars, and improve test_cross_build performance #1646

Merged

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Nov 9, 2024

Description

Cleans up some of the formatting of test/shell/test_{runner,scalafmt_helper}.sh and adds the RULES_SCALA_TEST_{ONLY,VERBOSE} environment variables. Improves test_cross_build.sh performance signficantly.

Defining RULES_SCALA_TEST_ONLYwill cause only one specific test case to run, with full Bazel output. This is helpful for pinpointing and working on a specific test without hunting for it or recreating its commands.

Defining RULES_SCALA_TEST_VERBOSE will cause all tests to emit full Bazel output. This is more useful when running all the tests from a specific script from test/shell/test_*.sh.

The test_cross_build.sh changes speed up its tests substantially, particularly between runs. Here are the times for two runs before this change; the first is after running bazel clean and bazel shutdown in the test_cross_version directory:

$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (73 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (94 sec)
      167.01 real         0.37 user         0.50 sys

$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (8 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (92 sec)
       99.96 real         0.35 user         0.48 sys

Here are the times for two equivalent runs after this change:

$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (71 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (6 sec)
       77.50 real         0.33 user         0.44 sys

$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (5 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (2 sec)
        7.21 real         0.26 user         0.38 sys

Motivation

The cleanup began as an attempt to get protobuf to stop recompiling so often in my new branch to use the latest abseil-cpp 20240722.0 and protobuf v28.3. It didn't impact that problem, but they were good cleanups regardless. (Though today I learned that SECONDS is a special Bash variable, and marking it local is a mistake that makes all tests look like they ran in zero seconds.)

The test_cross_version.sh speedups were also a result of working on that branch, on which the speedups described above were even more dramatic.

Previously, every bazel run in test_scalafmt would rebuild the entire protobuf library. I thought it was hanging, until I used RULES_SCALA_TEST_ONLY and could see what was happening. Then I used RULES_SCALA_TEST_VERBOSE to see the relationship between bazel invocations in test_cross_build.sh. That led me to remove the bazel clean from test_cross_build and move the bazel shutdown to the end of the script.

As always, this is spawned from work related to #1482, though it is a bit of a side quest.

Did a little cleaning up while trying to get protobuf to stop
recompiling so often. Didn't impact that problem, but good cleanups
regardless.

Also added two environment variables to make diagnosing test failures
(or slowdowns) easier:

- Defining `RULES_SCALA_TEST_ONLY`will cause only one specific test case
  to run, with full Bazel output. This is helpful for pinpointing and
  working on a specific test without hunting for it or recreating its
  commands.

- Defining `RULES_SCALA_TEST_VERBOSE` will cause all tests to emit full
  Bazel output. This is more useful when running all the tests from a
  specific script from `test/shell/test_*.sh`.

Also, today I learned that `SECONDS` is a special Bash variable. Making
it `local` screwed it up and showed all tests as taking zero seconds.
This speeds up the `test_cross_build.sh` tests substantially,
particularly between runs. Here are the times for two runs before this
change; the first is after running `bazel clean` and `bazel shutdown` in
the `test_cross_build` directory:

```txt
$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (73 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (94 sec)
      167.01 real         0.37 user         0.50 sys

$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (8 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (92 sec)
       99.96 real         0.35 user         0.48 sys
```

Here are the times for two equivalent runs after this change:

```txt
$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (71 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (6 sec)
       77.50 real         0.33 user         0.44 sys

$ /usr/bin/time ./test_cross_build.sh

running test test_cross_build
 Test "test_cross_build" successful (5 sec)
running test test_scalafmt
 Test "test_scalafmt" successful (2 sec)
        7.21 real         0.26 user         0.38 sys
```

The effect is even more dramatic when setting compiler flags in
`.bazelrc`, as I've tried while building protobuf-v28.3 under Bazel
6.5.0.
Copy link
Collaborator

@simuons simuons left a comment

Choose a reason for hiding this comment

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

Thanks!

@liucijus liucijus merged commit d43ae0d into bazelbuild:master Nov 12, 2024
2 checks passed
@mbland mbland deleted the bzlmod-cleanup-test-runner-add-env-vars branch November 12, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants