Skip to content

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

Merged
aignas merged 10 commits intobazel-contrib:mainfrom
trentontrees:fix-coverage-lcov
Feb 5, 2024
Merged

fix(coverage): generating lcov was causing issues#1734
aignas merged 10 commits intobazel-contrib:mainfrom
trentontrees:fix-coverage-lcov

Conversation

@trentontrees
Copy link
Copy Markdown
Contributor

@trentontrees trentontrees commented Jan 30, 2024

  • 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

* 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.
Copy link
Copy Markdown
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?

@aignas
Copy link
Copy Markdown
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?

* `bazel coverage ...` in the same fashion as `bazel test ...`
* update the python_bootstrap_template to default /dev/null for stdout/stderr
@trentontrees
Copy link
Copy Markdown
Contributor Author

trentontrees commented Feb 1, 2024

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 //...

* reverted presubmit 6.4.0 bazel ubuntu test case
* added changelog information
* remove setting stdin to /dev/null
@aignas
Copy link
Copy Markdown
Collaborator

aignas commented Feb 2, 2024

Let me know when this becomes ready to review again.

@trentontrees
Copy link
Copy Markdown
Contributor Author

@aignas , this should be ready for review.

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

Added the comments, hopefully they bring value ;)

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

@trentontrees
Copy link
Copy Markdown
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 bazel-contrib:main with commit ebbcb6a Feb 5, 2024
@aignas aignas mentioned this pull request Feb 5, 2024
@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 added a commit to phst/rules_elisp that referenced this pull request Feb 16, 2024
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