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

fix(coverage): generating lcov was causing issues #1734

Merged
merged 10 commits into from Feb 5, 2024
Merged

fix(coverage): generating lcov was causing issues #1734

merged 10 commits into from Feb 5, 2024

Conversation

trentontrees
Copy link
Contributor

@trentontrees trentontrees commented Jan 30, 2024 •
edited

  • examples/bzlmod - bazel coverage //tests:version_3_10_takes_3_9_subprocess_test was always failing due to .coveragerc generated file not being unique
  • generating the locv report resulted in messages going to stdout/stderr that resulted in test failures. To fix this, we run with --quiet. If VERBOSE_COVERAGE is defined we will output to stderr.

Reproduction steps:

git co main
cd examples/bzlmod
bazel coverage //tests/...
# failures occur

@trentontrees
fix(coverage): generating lcov was causing issues
f3559d5 
* examples/bzlmod - bazel coverage //tests:version_3_10_takes_3_9_subprocess_test was always failing due to .coveragerc generated file not being unique
* generating the locv report resulted in messages going to stdout/stderr that resulted in test failures.  To fix this, we run with --quiet.  If VERBOSE_COVERAGE is defined we will output to stderr.
@trentontrees trentontrees mentioned this pull request Jan 30, 2024
Merged
aignas
aignas reviewed Jan 31, 2024
View reviewed changes
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this will work only with bazel 7.0, could you add an item in the CHANGELOG.md in the fixed section so that we communicate this to users?

python/private/python_bootstrap_template.txt Show resolved Hide resolved
@aignas
Copy link
Collaborator

aignas commented Jan 31, 2024

Could you also update https://github.com/bazelbuild/rules_python/blob/main/.bazelci/presubmit.yml#L62 and related so that we can reproduce the failures in the CI?

@trentontrees
fix: verify coverage works the same as test
b996053 
* `bazel coverage ...` in the same fashion as `bazel test ...`
@trentontrees
Merge branch 'break-bazelci' into fix-coverage-lcov
0508403
@trentontrees
fix: lcov stdout/err => /dev/null
0879d3f 
* update the python_bootstrap_template to default /dev/null for stdout/stderr
@trentontrees
Copy link
Contributor Author

trentontrees commented Feb 1, 2024 •
edited

I updated the presubmit.yml to make the failing tests run. I'm having troubles reproducing the Ubuntu 20.04 issues locally so I can debug, iterate, and fix it.

I created a separate PR to show the issue without any changes in this PR. #1740

** UPDATE **
I'm able to reproduce the issue locally with:

USE_BAZEL_VERSION=6.4.0 bazel coverage //...

@trentontrees
WIP: see if presubmit passes if we skip lcov
e27a64e
@trentontrees
during lcov generation, set stdin to /dev/null
846df89
@trentontrees
fix: presubmit.yml
d284eb5 
* reverted presubmit 6.4.0 bazel ubuntu test case
* added changelog information
* remove setting stdin to /dev/null
@aignas
Copy link
Collaborator

aignas commented Feb 2, 2024

Let me know when this becomes ready to review again.

@trentontrees
Copy link
Contributor Author

@aignas , this should be ready for review.

aignas
aignas reviewed Feb 2, 2024
View reviewed changes
python/private/python_bootstrap_template.txt
params = [python_program, coverage_entrypoint, "lcov", "--rcfile=" + rcfile_name, "-o", output_filename, "--quiet"]
kparams = {"env": env, "cwd": workspace, "stdout": subprocess.DEVNULL, "stderr": subprocess.DEVNULL}
if IsVerboseCoverage():
params.remove("--quiet")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think adding a comment into why this behaviour is as is would be great.

@trentontrees
Copy link
Contributor Author

Added the comments, hopefully they bring value ;)

Rewrote the CHANGELOG.md blurb while I was at it.

@trentontrees
Copy link
Contributor Author

@aignas This should be ready for review again.

@aignas aignas added this pull request to the merge queue Feb 5, 2024
Merged via the queue into bazelbuild:main with commit ebbcb6a Feb 5, 2024
4 checks passed
@aignas aignas mentioned this pull request Feb 5, 2024
Closed
@trentontrees trentontrees deleted the fix-coverage-lcov branch February 5, 2024 17:02
phst added a commit to phst/rules_elisp that referenced this pull request Feb 16, 2024
@phst
Revert "Work around a potential Bazel bug related to Python coverage"
c4d68d2 
This reverts commit 3585d81.

The fix (bazelbuild/rules_python#1734,
bazelbuild/rules_python@ebbcb6a)
is included in rules_python now.
@phst phst mentioned this pull request Feb 16, 2024
Merged
phst added a commit to phst/rules_elisp that referenced this pull request Feb 16, 2024
@phst
Revert "Work around a potential Bazel bug related to Python coverage"
e61268f 
This reverts commit 3585d81.

The fix (bazelbuild/rules_python#1734,
bazelbuild/rules_python@ebbcb6a)
is included in rules_python now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aignas aignas aignas approved these changes

@rickeylev rickeylev Awaiting requested review from rickeylev rickeylev is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@trentontrees @aignas