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

Throw explicit exception if experimental_split_coverage_postprocessing is used without experimental_fetch_all_coverage_outputs #16140

Conversation

blindpirate
Copy link
Contributor

This closes #13185
According to e411fa7,
The flag --experimental_split_coverage_postprocessing depends on the flag
--experimental_fetch_all_coverage_outputs being enabled, but if the former one
is set without the latter one, Bazel throws a quite confusing NullPointerException.
Now we throw an explicit exception with proper error message.

@blindpirate blindpirate force-pushed the fix-experimental_split_coverage_postprocessing branch 3 times, most recently from 2b2d3e1 to cb69580 Compare August 21, 2022 13:27
@blindpirate
Copy link
Contributor Author

@sgowroji The failure seems to be because CI machine is disconnected from Internet, can you give me some hints how to handle external dependencies which need to be downloaded in integration tests?

Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

LGTM

@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 16, 2022
@gtech-bazel-bot gtech-bazel-bot bot added the awaiting-review PR is awaiting review from an assigned reviewer label Sep 27, 2022
@oquenchil
Copy link
Contributor

Adding @c-mita

Copy link
Member

@c-mita c-mita left a comment

Choose a reason for hiding this comment

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

The test setup can be greatly simplified; it should be unnecessary to pull in the rules_cc stuff.

#include "hello-greet.h"

TEST(HelloTest, GetGreet) {
EXPECT_EQ(get_greet("Bazel"), "Hello Bazel");
Copy link
Member

Choose a reason for hiding this comment

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

This could just be return 0; or something - the actual test result doesn't matter, and we don't want to try pulling in gtest.

@@ -111,13 +111,10 @@ public TestRunnerSpawn createTestRunnerSpawn(
throws ExecException, InterruptedException {
if (action.getExecutionSettings().getInputManifest() == null) {
String errorMessage = "cannot run local tests with --nobuild_runfile_manifests";
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this now unused local variable.

Comment on lines 918 to 935
cat << EOF > hello-greet.h
#ifndef LIB_HELLO_GREET_H_
#define LIB_HELLO_GREET_H_

#include <string>

std::string get_greet(const std::string &thing);

#endif
EOF

cat << EOF > hello-greet.cc
#include <string>

std::string get_greet(const std::string& who) {
return "Hello " + who;
}
EOF
Copy link
Member

Choose a reason for hiding this comment

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

If you actually want a file under test, there's no need to make it complicated; a simple int foo() { return 42; } is more than sufficient here; we really don't care about the coverage reports, only that the flag combination raises an error,

@blindpirate blindpirate force-pushed the fix-experimental_split_coverage_postprocessing branch from cb69580 to bd105cb Compare October 5, 2022 03:14
…g is used without experimental_fetch_all_coverage_outputs

This closes bazelbuild#13185
According to bazelbuild@a445cda,
The flag --experimental_split_coverage_postprocessing depends on the flag
--experimental_fetch_all_coverage_outputs being enabled, but if the former one
is set without the latter one, Bazel throws a quite confusing NullPointerException.
Now we throw an explicit exception with proper error message.
@blindpirate blindpirate force-pushed the fix-experimental_split_coverage_postprocessing branch from bd105cb to 73a8d6c Compare October 5, 2022 03:15
@blindpirate
Copy link
Contributor Author

Hi @sgowroji @oquenchil @c-mita now it's all green, PTAL!

@copybara-service copybara-service bot closed this in 49eb31b Oct 6, 2022
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
…g is used without experimental_fetch_all_coverage_outputs

This closes bazelbuild#13185
According to bazelbuild@e411fa7,
The flag --experimental_split_coverage_postprocessing depends on the flag
--experimental_fetch_all_coverage_outputs being enabled, but if the former one
is set without the latter one, Bazel throws a quite confusing NullPointerException.
Now we throw an explicit exception with proper error message.

Closes bazelbuild#16140.

PiperOrigin-RevId: 479239693
Change-Id: I5afa0ae14a3f34a0a7b21fbf4badad3d1836da95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using --experimental_split_coverage_postprocessing causes a RuntimeException
3 participants