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.

Sign up for GitHub

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

Jump to bottom

Add new option --treat_no_test_targets_found_as_success_return_code #11465

Closed
davido opened this issue May 22, 2020 · 4 comments
Closed

Add new option --treat_no_test_targets_found_as_success_return_code #11465

davido opened this issue May 22, 2020 · 4 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@davido
Copy link
Contributor

davido commented May 22, 2020 •
edited
Loading

Right now the code unconditionally returns exit code 4 in this situation, see: https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/runtime/commands/TestCommand.java#L165-L187

The problem is, that this requires some boilerplate on the CI code to differentiate between 0, 1 and 4. Gerrit Code Review has 100+ plugins, and we don't have plugin specific CI configuration, so that we would like to run generic command:

  $ bazelisk test plugins/{name}/...

The problem is to handle NO_TEST_TARGETS outcome on the CI side:

echo 'Running tests...'
set +e
bazelisk test plugins/{name}/...
TEST_RES=$?
set -e
if [ $TEST_RES -eq 4 ]
then
    echo 'No tests found for this plugin (tell this to the plugin maintainers?).'
elif [ ! $TEST_RES -eq 0 ]
then
    echo 'Tests failed'
    exit 1
fi

Instead, we would like to be able to just say this:

  $ bazelisk test --treat_no_test_targets_found_as_success_return_code \
     plugins/{name}/...

See this CL for more details: [1].

[1] https://gerrit-review.googlesource.com/c/gerrit-ci-scripts/+/268512
@ulfjack
Copy link
Contributor

ulfjack commented May 22, 2020

Some people unconditionally add an always-passing test to their command line as a workaround.

lucamilanesio pushed a commit to GerritCodeReview/gerrit-ci-scripts that referenced this issue May 22, 2020
@davido @lucamilanesio
Revisit plugin test target detection
9e5276c 
The way it is currently implemented to detect test targets is error
prone. In case of find-owners plugin there are two test targets:

$ grep -2 junit_tests BUILD | grep -o 'name = "[^"]*"' | cut -d '"' -f 2
findowners_junit_tests
findowners_IT_tests

But the code assumes that there are only one single target:

if [ "$TEST_TARGET" != "" ]
then
  bazelisk test --test_env DOCKER_HOST=$DOCKER_HOST plugins/{name}:$TEST_TARGET
fi

And this is why the second target findowners_IT_tests gets orphaned and
is not prefixed with "plugins/{name}" prefix and thus not found.

Moreover, this approach to grep for targets can also lead to false
negative results, because the test rules might be located in nested
plugin packages but not in the root package, like it is the case for
checks plugin. For such plugins no test are executed at all.

To rectify, avoid grep and just use Bazel's native wildcard "/..."
target feature, that should work in all three possible cases:

1. no test rule exist
2. one single test rule exsits in plugin's root package
3. many test rules exist in root and/or nested packages

This approach has problem with exit code of bazel in case that there are
no tests even thoufg test run was requested.

As documented in the user guide: [1], it is expected that Bazel
unconditionally returns exit code 4 in this situation. Check for that
exit code explicitly and warn CI users to contact plugin maintainers if
there were no tests found. Let's face it: the code is inherently broken
if there are no tests at all. Still we would like to avoid the
boilerplate to be needed and wrote this feature request: [2].

[1] https://docs.bazel.build/versions/master/guide.html#what-exit-code-will-i-get
[2] bazelbuild/bazel#11465

Bug: Issue 12512
Change-Id: Id641cfd2300939d3eac5adb4cdc4a14b12d5f7c2
@davido
Copy link
Contributor Author

davido commented May 22, 2020

Some people unconditionally add an always-passing test to their command line as a workaround.

Should the CI manipulate BUILD files for SUT to achieve that workaround or what exactly do you mean by: "to their command line"? Do you have code pointer for me?

@aiuto aiuto added P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions labels May 22, 2020
@aiuto
Copy link
Contributor

aiuto commented May 22, 2020

I believe @ulfjack's suggestion was to do

bazelisk test //:always_true_test //plugins/{name}/...

@davido It seems to me from your script that you are making use of the 4 result

echo 'No tests found for this plugin (tell this to the plugin maintainers?).'

I am not keen on a separate flag for changing the result. Instead I would like to see "no tests found" integrated with the work for skipping non-applicable test targets. So you would ask for a structured result file saying how many tests were found, skipped, passed, failed. Maybe even we break down skipped into skipped because of configuration non-applicability or skipped because of filtering.

@davido
Copy link
Contributor Author

davido commented May 22, 2020

Thanks for this suggestion.

@davido davido closed this as completed May 22, 2020
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue May 26, 2020
@davido
Bazel: Add always pass test to avoid boilerplate in the CI
d69773c 
We don't know whether or not all plugins have test rules, so running a
generic command:

  $ bazelisk test plugins/{name}/...

can lead to no tests found outcome, in which case Bazel will return exit
code 4 that needs to be checked to differentiate from test failure (exit
code 1). To avoid those checks, pass the virtual test that will always
succeed:

  $ bazelisk test //tools/bzl:always_pass_test plugins/{name}/...

See also this upstream issue for this suggested workaround: [1].

[1] bazelbuild/bazel#11465

Change-Id: Ideab64674482400cc48fad55f7ed9f8085909b84
lucamilanesio pushed a commit to GerritCodeReview/gerrit-ci-scripts that referenced this issue May 26, 2020
@davido @lucamilanesio
Avoid boilerplate in no tests found use case
a19af81 
As suggested in this upstream issue: [1] use always pass test to avoid
Bazel returning exit code 4 and corresponding boilerplate in no tests
found use case.

[1] bazelbuild/bazel#11465

Change-Id: I3b102d0efae69cf975e0e2787537efc5898d5938
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue Jun 6, 2020
@davido @lucamilanesio
Bazel: Add always pass test to avoid boilerplate in the CI
938db88 
We don't know whether or not all plugins have test rules, so running a
generic command:

  $ bazelisk test plugins/{name}/...

can lead to no tests found outcome, in which case Bazel will return exit
code 4 that needs to be checked to differentiate from test failure (exit
code 1). To avoid those checks, pass the virtual test that will always
succeed:

  $ bazelisk test //tools/bzl:always_pass_test plugins/{name}/...

See also this upstream issue for this suggested workaround: [1].

[1] bazelbuild/bazel#11465

Change-Id: Ideab64674482400cc48fad55f7ed9f8085909b84
(cherry picked from commit d69773c)
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue Jun 8, 2020
@davido @lucamilanesio
Bazel: Add always pass test to avoid boilerplate in the CI
f4da11a 
We don't know whether or not all plugins have test rules, so running a
generic command:

  $ bazelisk test plugins/{name}/...

can lead to no tests found outcome, in which case Bazel will return exit
code 4 that needs to be checked to differentiate from test failure (exit
code 1). To avoid those checks, pass the virtual test that will always
succeed:

  $ bazelisk test //tools/bzl:always_pass_test plugins/{name}/...

See also this upstream issue for this suggested workaround: [1].

[1] bazelbuild/bazel#11465

Change-Id: Ideab64674482400cc48fad55f7ed9f8085909b84
(cherry picked from commit d69773c)
(cherry picked from commit 938db88)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@davido @aiuto @ulfjack