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

Update test scripts for Bzlmod compatibility #1622

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Oct 15, 2024

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

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, looks good to me. Please take a look at my question. Once you confirm it's intentional I'll approve this PR.

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 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

mbland added a commit to mbland/rules_scala that referenced this pull request Oct 18, 2024
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
Copy link
Contributor Author

mbland commented Oct 18, 2024

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
Copy link
Contributor Author

mbland commented Oct 18, 2024

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.
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 mbland force-pushed the bzlmod-update-test-scripts branch from 33034d6 to c4b5a9a Compare October 21, 2024 14:53
@mbland
Copy link
Contributor Author

mbland commented Oct 21, 2024

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

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. Again, it's a pleasure to read descriptions of your PRs.

@liucijus liucijus merged commit 5369366 into bazelbuild:master Oct 23, 2024
2 checks passed
@mbland mbland deleted the bzlmod-update-test-scripts branch October 23, 2024 14:11
mbland added a commit to mbland/rules_scala that referenced this pull request Dec 11, 2024
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.
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