Update test scripts for Bzlmod compatibility #1622

mbland · 2024-10-15T01:29:46Z

Description

These are mostly small changes to make test assertions more flexible between WORKSPACE and Bzlmod runs.

For Bzlmod runs:

Fixed test_scala_config_content from test_scala_config.sh by changing a path from external/io_bazel_rules_scala_config to external/*io_bazel_rules_scala_config.
Fixed a number of tests by updating expected output messages to allow them to start with either @// or @@//.
Fixed test_stamped_target_label_loading from test/shell/test_strict_dependency.sh by accommodating the canonical io_bazel_rules_scala_guava repo name. Also allows for the optional current Scala version suffix.

Also made these other important changes:

Updated all the assertions in test_helper.sh to use Bash builtin regex matching via _expect_failure_with_messages instead of grep. This allows the expected message patterns to use full regular expressions while avoiding forking a new process. This new function helped reduce duplication in that file at the same time.
Added --repo_env="SCALA_VERSION=..." to each test script called from ./test_coverage.sh, and set SCALA_VERSION to 2.12.19 in each of these files. Using other Scala versions technically works, but the output is slightly different, causing the diff commands in the test cases to fail.
Updated test_version.sh to copy the top level .bazelversion file into its test repo.
Changed how test_version.sh handles injecting twitter_scrooge repos into the WORKSPACE file. This will make it easy to do the equivalent for MODULE.bazel when the time comes.

Also includes a few minor, opportunistic formatting cleanups.

Motivation

Part of adding Bzlmod support per #1482.

I'm breaking this out to make sure the current default build configuration continues to pass as I carve off more of my fork's Bzlmod working branch. At the same time, the later changes can land without including so many test changes.

cc: @BillyAutrey @jayconrod @benjaminp @TheGrizzlyDev

simuons

Thanks, @mbland, looks good to me. Please take a look at my question. Once you confirm it's intentional I'll approve this PR.

simuons · 2024-10-18T07:41:01Z

test/shell/test_coverage_scalatest.sh

    diff test/coverage_scalatest/expected-coverage.dat $(bazel info bazel-testlogs)/test/coverage_scalatest/test-scalatest/coverage.dat
 }

 test_coverage_includes_test_targets() {
    bazel coverage \
-          --instrument_test_targets=True \
-          //test/coverage_scalatest:test-scalatest
+        --instrument_test_targets=True \


Shouldn't --repo_env="SCALA_VERSION=${SCALA_VERSION}" be passed here as well?

Good catch! But I actually went the other way—the tests missing --repo_env="SCALA_VERSION=${SCALA_VERSION}" are only grepping for the expected filename, not diffing against an expected coverage file. So I've added comments in each file to explain the SCALA_VERSION setting.

I think the actual solution might be to generate coverage files for each Scala version, and use $SCALA_VERSION to select between them instead of setting --repo_env. I can take that on as a side quest, perhaps.

@simuons

After @simuons asked about an apparently missing `--repo_env` setting in `test_coverage_scalatest.sh` in bazelbuild#1622, I realized the test case missing it used `grep`, not `diff`. Rather than add the flag where it wasn't needed, I commented the local `SCALA_VERSION` setting in each file. Now that I think about it, the proper solution is to generate coverage files for each Scala version, or for ranges of scala versions. Then we can remove this `SCALA_VERSION` and `--repo_env` setting completely. I'll look into that as a side quest.

mbland · 2024-10-18T13:45:27Z

On another note, I'm going to open a new PR to try to bump rules_java to 7.11.1 to see if it resolves this persistent "Bazel green head" CI breakage:

bazel --nosystem_rc --nohome_rc info
ERROR: Failed to load Starlark extension '@@rules_java//toolchains:toolchain_utils.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @@rules_java
This could either mean you have to add the '@@rules_java' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: cannot load build configuration key because of this cycle
Traceback (most recent call last):
  File "bazelci.py", line 4588, in <module>
    sys.exit(main())
  File "bazelci.py", line 4556, in main
    execute_commands(
  File "bazelci.py", line 1321, in execute_commands
    bazel_version = print_bazel_version_info(bazel_binary, platform)
  File "bazelci.py", line 1623, in print_bazel_version_info
    execute_command(
  File "bazelci.py", line 2750, in execute_command
    return subprocess.run(
  File "/usr/lib/python3.8/subprocess.py", line 516, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['bazel', '--nosystem_rc', '--nohome_rc', 'info']' returned non-zero exit status 2.

mbland · 2024-10-18T14:01:43Z

Ah, just reminded myself of the problem with rules_java and WORKSPACE. Filed #1625 for a general discussion, so it doesn't get buried here.

Part of bazelbuild#1482. These are mostly small changes to make test assertions more flexible between `WORKSPACE` and Bzlmod runs. For Bzlmod runs: - Fixed `test_scala_config_content` from `test_scala_config.sh` by changing a path from `external/io_bazel_rules_scala_config` to `external/*io_bazel_rules_scala_config`. - Fixed a number of tests by updating expected output messages to allow them to start with either `@//` or `@@//`. - Fixed `test_stamped_target_label_loading` from `test/shell/test_strict_dependency.sh` by accommodating the canonical `io_bazel_rules_scala_guava` repo name. Also allows for the optional current Scala version suffix. Also made these other important changes: - Updated all the assertions in `test_helper.sh` to use Bash builtin regex matching via `_expect_failure_with_messages` instead of `grep`. This allows the expected message patterns to use full regular expressions while avoiding forking a new process. This new function helped reduce duplication in that file at the same time. - Added `--repo_env="SCALA_VERSION=..."` to each test script called from `./test_coverage.sh`, and set `SCALA_VERSION` to 2.12.19 in each of these files. Using other Scala versions technically works, but the output is slightly different, causing the `diff` commands in the test cases to fail. - Updated `test_version.sh` to copy the top level `.bazelversion` file into its test repo. - Changed how `test_version.sh` handles injecting `twitter_scrooge` repos into the `WORKSPACE` file. This will make it easy to do the equivalent for `MODULE.bazel` when the time comes. Also includes a few minor, opportunistic formatting cleanups.

@simuons

After @simuons asked about an apparently missing `--repo_env` setting in `test_coverage_scalatest.sh` in bazelbuild#1622, I realized the test case missing it used `grep`, not `diff`. Rather than add the flag where it wasn't needed, I commented the local `SCALA_VERSION` setting in each file. Now that I think about it, the proper solution is to generate coverage files for each Scala version, or for ranges of scala versions. Then we can remove this `SCALA_VERSION` and `--repo_env` setting completely. I'll look into that as a side quest.

mbland · 2024-10-21T15:21:54Z

Rebased and passing after #1627, including latest Bazel 7.3.2.

simuons

Thanks, @mbland. Again, it's a pleasure to read descriptions of your PRs.

Update tests for Bazel 6 + Bzlmod Copied the regular expression used to optionally match canonical repo name prefixes in `buildozer` commands from bazelbuild#1622. These never failed when building with Bzlmod and Bazel 7.4.1, but did under Bazel 6.5.0.

Copied the regular expression used to optionally match canonical repo name prefixes in `buildozer` commands from bazelbuild#1622. These never failed when building with Bzlmod and Bazel 7.4.1, but did under Bazel 6.5.0.

* Toolchainize //scala/scalafmt:scalafmt_toolchain This is a pretty straightforward and easy update on top of the previous `setup_toolchains` and `scala_toolchains_repo` changes. Part of #1482. * Make test_reproducibility.sh build test/coverage_* Replaces the nonexistent `//test/coverage/...` target with packages that actually exist, rendering the test more meaningful. A spurious failure caused me to run the following to inspect what was happening: ```txt RULES_SCALA_TEST_ONLY=test_build_is_identical ./test_reproducibility.sh ``` This caused me to see that the following command was failing because `//test/coverage/...` didn't exist: ```txt bazel build --collect_code_coverage -- //test/coverage/... ``` * Export `SCALAFMT_TOOLCHAIN_TYPE` constant Reduces string duplication in `//scala/scalafmt/toolchain/*.bzl` files. * Remove scalafmt_toolchain dep_providers default It's likely that no one will ever rely on the default when defining their own `scalafmt_toolchain`. While investigating #1675, I realized the `dep_providers` default was set to a nonexistent target. This didn't break our tests because our Scalafmt toolchains are created by `setup_scala_toolchain`, which sets `dep_providers` explicitly. I thought about aliasing the provider generated for the toolchain for `SCALA_VERSION` in `@io_bazel_rules_scala_toolchains//scalafmt`, or generating a new one. I ultimately decided to remove the default, because the `deps_providers` is literally the only attribute. Anyone defining their own `scalafmt_toolchain` will certainly define their own `deps_providers` target(s). * Update tests for Bazel 6 + Bzlmod Copied the regular expression used to optionally match canonical repo name prefixes in `buildozer` commands from #1622. These never failed when building with Bzlmod and Bazel 7.4.1, but did under Bazel 6.5.0. * Fix `//third_party` tests with `$(rootpath)` Fixes `{strict_deps,unused_dependency_checker}_test` from `//third_party/dependency_analyzer/src/test` under Bazel 8 by updating `$(location)` to `$(rootpath)`. Part of #1652. `//third_party/dependency_analyzer/src/test:strict_deps_test` and `//third_party/dependency_analyzer/src/test:unused_dependency_checker_test` both failed with errors of the form: ```txt StrictDepsTest: - error on indirect dependency target *** FAILED *** (379 milliseconds) nice errors on sequence of strings.this.infos.exists(nice errors on sequence of strings.this.checkErrorContainsMessage(target)) was false expected an error on //commons:Target to appear in errors (with buildozer command)! Errors: List(object apache is not a member of package org) (StrictDepsTest.scala:85) ``` This happened both under `WORKSPACE` and Bzlmod. Copying the test script with the following (with `$ID` being one of `7`, `8`, `7-updated`, or `8-updated`) helped reveal the differences: ```txt cp bazel-bin/third_party/dependency_analyzer/src/test/strict_deps_test \ strict_deps_test-$ID.sh ``` Under Bazel 7, we find the following lines whether using `$(location)` or `$(rootpath)` on the `org.apache.commons` artifact (the other artifacts already use `$(rootpath)`: ```txt -Dscala.library.location=external/io_bazel_rules_scala_scala_library_2_12_20/scala-library-2.12.20.jar -Dscala.reflect.location=external/io_bazel_rules_scala_scala_reflect_2_12_20/scala-reflect-2.12.20.jar -Dguava.jar.location=external/com_google_guava_guava_21_0_with_file/guava-21.0.jar -Dapache.commons.jar.location=external/org_apache_commons_commons_lang_3_5_without_file/commons-lang3-3.5.jar ``` Under Bazel 8, notice that the `external/` prefix has become `../` for the other paths already using `$(rootpath)`, but remains `external/` for the `$(location)` artifact. This breaks the Bazel 8 build: ```txt -Dscala.library.location=../io_bazel_rules_scala_scala_library_2_12_20/scala-library-2.12.20.jar -Dscala.reflect.location=../io_bazel_rules_scala_scala_reflect_2_12_20/scala-reflect-2.12.20.jar -Dguava.jar.location=../com_google_guava_guava_21_0_with_file/guava-21.0.jar -Dapache.commons.jar.location=external/org_apache_commons_commons_lang_3_5_without_file/commons-lang3-3.5.jar ``` Under Bazel 8, using `$(rootpath)` for the `org.apache.commons` artifact converts its `external/`, fixing the Bazel 8 build: ```txt -Dscala.library.location=../io_bazel_rules_scala_scala_library_2_12_20/scala-library-2.12.20.jar -Dscala.reflect.location=../io_bazel_rules_scala_scala_reflect_2_12_20/scala-reflect-2.12.20.jar -Dguava.jar.location=../com_google_guava_guava_21_0_with_file/guava-21.0.jar -Dapache.commons.jar.location=../org_apache_commons_commons_lang_3_5_without_file/commons-lang3-3.5.jar ```

mbland requested review from liucijus and simuons as code owners October 15, 2024 01:29

simuons requested changes Oct 18, 2024

View reviewed changes

mbland added 2 commits October 21, 2024 10:53

mbland force-pushed the bzlmod-update-test-scripts branch from 33034d6 to c4b5a9a Compare October 21, 2024 14:53

mbland mentioned this pull request Oct 21, 2024

Support Bzlmod and add rules_scala to bazel-central-registry #1482

Open

simuons approved these changes Oct 23, 2024

View reviewed changes

liucijus approved these changes Oct 23, 2024

View reviewed changes

liucijus merged commit 5369366 into bazelbuild:master Oct 23, 2024
2 checks passed

mbland deleted the bzlmod-update-test-scripts branch October 23, 2024 14:11

mbland mentioned this pull request Nov 10, 2024

Protobuf compatibility findings and suggestions #1647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update test scripts for Bzlmod compatibility #1622

Update test scripts for Bzlmod compatibility #1622

mbland commented Oct 15, 2024

simuons left a comment

simuons Oct 18, 2024

mbland Oct 18, 2024

mbland commented Oct 18, 2024

mbland commented Oct 18, 2024

mbland commented Oct 21, 2024

simuons left a comment

Update test scripts for Bzlmod compatibility #1622

Update test scripts for Bzlmod compatibility #1622

Conversation

mbland commented Oct 15, 2024

Description

Motivation

simuons left a comment

Choose a reason for hiding this comment

simuons Oct 18, 2024

Choose a reason for hiding this comment

mbland Oct 18, 2024

Choose a reason for hiding this comment

mbland commented Oct 18, 2024

mbland commented Oct 18, 2024

mbland commented Oct 21, 2024

simuons left a comment

Choose a reason for hiding this comment