Skip to content

Fix setup_scala_toolchains, clean up test scripts #1742

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

Merged
merged 2 commits into from
May 26, 2025

Conversation

mbland
Copy link
Contributor

@mbland mbland commented May 25, 2025

Description

Adds a Label() wrapper to setup_scala_toolchains() and scala_cross_version_select() to fix @rules_scala_config invisibilty build breakages under Bzlmod. Enables Bash strict mode for all test scripts via set -euo pipefail and fixes the resulting breakages.

Removes use_repo(scala_config, "rules_scala_config") from dt_patches/test_dt_patches{,_user_srcjar}/MODULE.bazel. This caused dt_patches/dt_patch_test.sh to break before adding the Label() wrapper to setup_scala_toolchains().

Also updates dt_patches/dt_patch_test.sh to use test/shell/test_runner.sh and test/shell/test_helper.sh. Replaces previous instances of its own run_test_local with the result of get_test_runner. Updates run_in_test_repo argument parsing, updates run_in_test_repo calls to use proper string quoting, and ensures any failures exit the script.

Other general test updates include:

  • Adds a new dt_patches_linux job to .bazelci/presubmit.yml to run dt_patches/dt_patch_test.sh as part of the CI build.

  • Sets set -euo pipefail in test_runner.sh and test_helper.sh and fixes all unbound variables and pipe failures. Updates test helpers in those files to perform better argument quoting, argument pass through, command execution, and output printing.

  • Updates test helpers in test_runner.sh and test_helper.sh to show more information when the new verbose_test_output helper returns success, even when a test passes. Removes exit 0 everywhere so successful tests return control to run_test_local when verbose_test_output is in effect. Adds an exit in run_test_local when RULES_SCALA_TEST_ONLY is nonempty.

  • Adds #!/usr/bin/env bash to a number of test files. This avoids having some test helper echo statements from printing the -e argument when running the test file directly. (As opposed to running them via test_all.sh or test_rules_scala.sh.)

  • Adds test/shell/test_macros.sh to test_rules_scala.sh.

Updates that fixed previously incorrect behavior of specific tests include:

  • Fixes test_scala_import_source_jar.sh after Bash strict mode fixes revealed underlying breakages. Specifically, if [[ $1 ]] in assert_file_exists() always returned true, which meant the function always returned true, because the src jar never existed. Also, the expected repo path was never valid under Bzlmod, since it relied on the WORKSPACE specific repo path naming convention. Changing the condition to if [[ -n "$1" ]] fulfilled the actual intention, and revealed the broken repo path and fetch_sources never being true.

    Ensures fetch_sources is True for @com_google_guava_guava_21_0 from //scala/private/extensions/dev_deps.bzl%dev_deps. Also makes the directory path to the repo Bzlmod compatible. (Fortunately, the underlying behavior the test intended to validate was already correct.)

  • Fixes test_scala_classpath.sh to actually grep for the expected string. Previously is was grepping for the empty string, thanks to the unbound $method variable, and always passed. Fixing this revealed that the test would fail when run if the test target artifacts already existed. Updated it to remove the test target artifacts so that the test could run correctly every time. (Again, the underlying behavior was already correct.)

Test updates resulting from Bash strict mode that didn't fundamentally change test behavior include:

  • Fixed the unbound $target_bes variable in test_build_event_protocol.sh by replacing it with ${bes_file}. The original intention appeared to be ${target}_bes.txt, and the test previously just happened to work by using a file named .txt.

  • Refactored test_invalid_scalacopts.sh to use action_should_fail_with{,out}_message. This resolves the previous pipe failure after turning on Bash strict mode.

  • Updates test_macros.sh to use Bash stderr redirection syntax. Overlaps with a change in Check for 'deps' attr in scala_doc rule #1740, but shouldn't conflict.

  • All other test updates defined previously unbound variables and fixed improper argument quotation.

Motivation

This contains an easy fix for a basic Bzlmod breakage. In looking to provide a test for it, I went down the path of improving all the shell tests by applying Bash strict mode. This turned into a larger pull request than I intended; as always, I'm happy to split it up if so desired.

Vince Rose reported the setup_scala_toolchains breakage under Bzlmod in the #scala channel of the Bazel Slack Workspace on 2025-05-21:

dt_patches/dt_patch_test.sh would've caught it had I not added use_repo(scala_config, "rules_scala_config") to the MODULE.bazel files. Removing these use_repo calls reproduced the error, and they passed again once I reintroduced the proper Label instances.

Rather than add another test, I thought it was time to ensure dt_patch_test.sh behaved properly and could run in CI. Also, @jayconrod recently reminded me of Bash strict mode during the course of an internal code review. So I added Bash strict mode to dt_patch_test.sh and refactored it to ensure that a test failure properly exited the script.

That went well, so I decided to set Bash strict mode in test_runner.sh and test_helper.sh. Hilarity ensued.

Having Bash strict mode in place should greatly reduce the chances of adding tests that pass without actually doing what we think they're doing. Several of the updates show more info when
RULES_SCALA_TEST_ONLY and RULES_SCALA_TEST_VERBOSE are in effect. This helped me immensely while getting to the root of these problems, especially when defining a previously unbound variable and having a test immediately pass. These RULES_SCALA_TEST_* variables helped me verify that they were really passing, or accidentally getting by.

There's still opportunities to refactor some of the tests to use more of the existing test helpers. I could do more test script cleanup passes in one or more future pull requests.

Adds a `Label()` wrapper to `setup_scala_toolchains()` and
`scala_cross_version_select()` to fix `@rules_scala_config` invisibilty
build breakages under Bzlmod. Enables Bash strict mode for all test
scripts via `set -euo pipefail` and fixes the resulting breakages.

Removes `use_repo(scala_config, "rules_scala_config")` from
`dt_patches/test_dt_patches{,_user_srcjar}/MODULE.bazel`. This caused
`dt_patches/dt_patch_test.sh` to break before adding the `Label()`
wrapper to `setup_scala_toolchains()`.

Also updates `dt_patches/dt_patch_test.sh` to use
`test/shell/test_runner.sh` and `test/shell/test_helper.sh`. Replaces
previous instances of its own `run_test_local` with the result of
`get_test_runner`. Updates `run_in_test_repo` argument parsing, updates
`run_in_test_repo` calls to use proper string quoting, and ensures any
failures exit the script.

Other general test updates include:

- Adds a new `dt_patches_linux` job to `.bazelci/presubmit.yml` to run
  `dt_patches/dt_patch_test.sh` as part of the CI build.

- Sets `set -euo pipefail` in `test_runner.sh` and `test_helper.sh` and
  fixes all unbound variables and pipe failures. Updates test helpers in
  those files to perform better argument quoting, argument pass through,
  command execution, and output printing.

- Updates test helpers in `test_runner.sh` and `test_helper.sh` to
  show more information when the new `verbose_test_output` helper
  returns success, even when a test passes. Removes `exit 0` everywhere
  so successful tests return control to `run_test_local` when
  `verbose_test_output` is in effect. Adds an exit in `run_test_local`
  when `RULES_SCALA_TEST_ONLY` is nonempty.

- Adds `#!/usr/bin/env bash` to a number of test files. This avoids
  having some test helper `echo` statements from printing the `-e`
  argument when running the test file directly. (As opposed to running
  them via `test_all.sh` or `test_rules_scala.sh`.)

- Adds `test/shell/test_macros.sh` to `test_rules_scala.sh`.

Updates that fixed previously incorrect behavior of specific tests
include:

- Fixes `test_scala_import_source_jar.sh` after Bash strict mode fixes
  revealed underlying breakages. Specifically, `if [[ $1 ]]` in
  `assert_file_exists()` always returned true, which meant the function
  always returned true, because the src jar never existed. Also, the
  expected repo path was never valid under Bzlmod, since it relied on
  the `WORKSPACE` specific repo path naming convention. Changing the
  condition to `if [[ -n "$1" ]]` fulfilled the actual intention, and
  revealed the broken repo path and `fetch_sources` never being true.

  Ensures `fetch_sources` is `True` for `@com_google_guava_guava_21_0`
  from `//scala/private/extensions/dev_deps.bzl%dev_deps`. Also makes
  the directory path to the repo Bzlmod compatible. (Fortunately, the
  underlying behavior the test intended to validate was already
  correct.)

- Fixes `test_scala_classpath.sh` to actually grep for the `expected`
  string. Previously is was grepping for the empty string, thanks to the
  unbound `$method` variable, and always passed. Fixing this revealed
  that the test would fail when run if the test target artifacts already
  existed. Updated it to remove the test target artifacts so that the
  test could run correctly every time. (Again, the underlying behavior
  was already correct.)

Test updates resulting from Bash strict mode that didn't fundamentally
change test behavior include:

- Fixed the unbound `$target_bes` variable in
  `test_build_event_protocol.sh` by replacing it with `${bes_file}`. The
  original intention appeared to be `${target}_bes.txt`, and the test
  previously just happened to work by using a file named `.txt`.

- Refactored `test_invalid_scalacopts.sh` to use
  `action_should_fail_with{,out}_message`. This resolves the previous
  pipe failure after turning on Bash strict mode.

- Updates `test_macros.sh` to use Bash stderr redirection syntax.
  Overlaps with a change in bazel-contrib#1740, but shouldn't conflict.

- All other test updates defined previously unbound variables and
  fixed improper argument quotation.

---

This contains an easy fix for a basic Bzlmod breakage. In looking to
provide a test for it, I went down the path of improving _all_ the shell
tests by applying Bash strict mode. This turned into a larger pull
request than I intended; as always, I'm happy to split it up if so
desired.

Vince Rose reported the `setup_scala_toolchains` breakage under Bzlmod
in the #scala channel of the Bazel Slack Workspace on 2025-05-21:

- https://bazelbuild.slack.com/archives/CDCKJ2KFZ/p1747870770592779

`dt_patches/dt_patch_test.sh` would've caught it had I not added
`use_repo(scala_config, "rules_scala_config")` to the `MODULE.bazel`
files. Removing these `use_repo` calls reproduced the error, and they
passed again once I reintroduced the proper `Label` instances.

Rather than add another test, I thought it was time to ensure
`dt_patch_test.sh` behaved properly and could run in CI. Also,
@jayconrod recently reminded me of Bash strict mode during the course of
an internal code review. So I added Bash strict mode to
`dt_patch_test.sh` and refactored it to ensure that a test failure
properly exited the script.

That went well, so I decided to set Bash strict mode in `test_runner.sh`
and `test_helper.sh`. Hilarity ensued.

Having Bash strict mode in place should greatly reduce the chances of
adding tests that pass without actually doing what we think they're
doing. Several of the updates show more info when
`RULES_SCALA_TEST_ONLY` and `RULES_SCALA_TEST_VERBOSE` are in effect.
This helped me immensely while getting to the root of these problems,
especially when defining a previously unbound variable and having a test
immediately pass. These `RULES_SCALA_TEST_*` variables helped me verify
that they were _really_ passing, or accidentally getting by.

There's still opportunities to refactor some of the tests to use more of
the existing test helpers. I could do more test script cleanup passes
in one or more future pull requests.
@mbland mbland requested review from liucijus and simuons as code owners May 25, 2025 15:00
`test_persistent_worker_handles_exception_in_macro_invocation` failed on
macOS for bazel-contrib#1742:

- https://buildkite.com/bazel/rules-scala-scala/builds/5620#01970821-a4f1-4333-9fe6-521679bf0a98

Not sure why it failed exactly, but I took the opportunity to refactor
it for `verbose_test_output` visibility. Seems to pass fine locally on
my macOS machine.

Updated `assert_matches` to take an optional `msg` argument to use it in
`test_persistent_worker.sh` test cases.

There's more opportunity to refactor here, but holding off for now.
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, @mbland, for yet another improvement!

@simuons simuons merged commit d222569 into bazel-contrib:master May 26, 2025
1 check passed
@mbland mbland deleted the fix-setup-scala-toolchain branch May 26, 2025 17:28
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.

2 participants