Skip to content

Conversation

@BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Feb 12, 2025

This ensures that un-executed files (i.e. files that aren't tested) are included in the coverage report. The current behavior is that coverage.py excludes them by default.

This PR configures source files via the auto-generated .coveragerc file.

See https://coverage.readthedocs.io/en/7.6.10/source.html#execution:

If the source option is specified, only code in those locations will be measured. Specifying the source option also enables coverage.py to report on un-executed files, since it can search the source tree for files that haven’t been measured at all.

Closes #2599
Closes #2597
Fixes #2575

@BurnzZ BurnzZ changed the title feat(coverage): add --source flag to coveragepy call feat(coverage): add --source flag to coveragepy call Feb 12, 2025
@aignas
Copy link
Collaborator

aignas commented Feb 13, 2025

@groodt
Copy link
Collaborator

groodt commented Feb 13, 2025

This might be a really interesting thing in the context of https://docs.google.com/document/d/1D_y9PAimvn566VyeLDu-FZLKrAcv48CL6ftKU03_dKs/edit?tab=t.0#heading=h.5mcn15i0e1ch

I think it's a valuable fix that should be merged. Even if/when the above Bazel issue comes to be, we can port or adopt this code towards that or replace it with upstream bazel capabilities.

@groodt groodt self-requested a review February 13, 2025 21:46
Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

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

LGTM

@BurnzZ BurnzZ changed the title feat(coverage): add --source flag to coveragepy call fix(coverage): Fix missing files in the coverage report if they have no tests Feb 13, 2025
@BurnzZ BurnzZ changed the title fix(coverage): Fix missing files in the coverage report if they have no tests fix(coverage): missing files in the coverage report if they have no tests Feb 13, 2025
@BurnzZ BurnzZ force-pushed the coveragepy-source branch 2 times, most recently from e9153e4 to 7ec81bd Compare February 13, 2025 22:48
@BurnzZ BurnzZ marked this pull request as ready for review February 13, 2025 22:48
@aignas
Copy link
Collaborator

aignas commented Feb 13, 2025

Could also add the same fix in the stage2 bootsrap code please?

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Feb 14, 2025

Good call. Updated stage2_bootstrap_template.py as well.

Can you also point me to the test suite for these files? I can't seem to find specific tests regarding coverage.

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

LGTM in concept.

My only concern is argv length for the system python bootstrap -- isn't this going to dramatically increase it? The argv length is already a source of issues. Can the paths be passed in via a file e.g. the rcfile, instead?

@rickeylev
Copy link
Collaborator

For tests...hm, we don't seem to have much in this area.

I think your best bet is to use sh_py_run_test and/or py_reconfig_test -- see tests/bootstrap_impls for example. These let you set arbitrary flags when building a py binary/test and run it via an sh_test. $COVERAGE_DIR/pylcov.dat should have the output.

You'll need to modify the py_reconfig rule to allow enabling coverage mode. Something like this, in tests/support/sh_py_run_test.bzl:

  • Add "//command_line_option:collect_code_coverage" to RECONFIG_INPUTS, RECONFIG_OUTPUTS
  • Add collect_code_coverage attr.string() to RECONFIG_ATTRS
  • Modify _perform_transition_impl:
    def _perform_transition_impl(...):
      ...
      if attr.collect_code_coverage:
        settings["//command_line_option:collect_code_coverage"] = attr.collect_code_coverage
    

You should then be able to use py_reconfig_test(collect_code_coverage="true") (or sh_py_run_test), and the target will mostly act as if bazel coverage is being run. You might have to manually set some of the COVERAGE_* environment variable; not sure. I've used this trick for analysis tests, but not full execution tests, so not sure what the env looks like.

For bootstrap=script, you can probably create more of a unittest by stashing the created Coverage object somewhere. Set an environment variable via env, then check for it in stage2_bootstrap. If set, stash the Coverage object somewhere (on something global, _bazel_site_init maybe)? You can then inspect it in the test to check what source paths are being used.

BurnzZ added 3 commits March 5, 2025 10:52
…ests

This ensures that un-executed files (i.e. files that aren't tested) are
included in the coverage report. The current behavior is that
coverage.py excludes them by default.

See https://coverage.readthedocs.io/en/7.6.10/source.html#execution
@BurnzZ BurnzZ force-pushed the coveragepy-source branch from b75eb7b to 76786dc Compare March 5, 2025 01:24
@BurnzZ
Copy link
Contributor Author

BurnzZ commented Mar 5, 2025

Cheers for the tips @rickeylev. Updated the PR to use the .coveragerc instead of passing to argv.

I got some trouble in adding //command_line_option:collect_code_coverage as I get:

Error in rule: attribute name `collect_code_coverage ` is not a valid identifier.

It doesn't seem to be present in the builtin_exec_platforms.bzl definitions. (I'm not sure if I'm interpreting it correctly that it's the source of //command_line_option)

The closest would be //command_line_option:experimental_collect_code_coverage_for_generated_files but I get this error when I use that instead:

Error in transition: Invalid transition input '//command_line_option:experimental_collect_code_coverage_for_generated_files'. Cannot transition on --experimental_* or --incompatible_* options

In the meantime, I've tested it manually via https://github.com/BurnzZ/bazel-python-omitted-coverage. Also inspected the generated .coveragerc file to ensure the contents are correct.

@aignas
Copy link
Collaborator

aignas commented Mar 6, 2025

Interesting that you are getting this error. We are matching on it here: https://github.com/bazelbuild/rules_python/blob/a816962e509311c23230730b4b28f9d52a229949/python/private/hermetic_runtime_repo_setup.bzl#L192

Not sure if this sparks any new ideas, but wanted to add the code reference.

@aignas
Copy link
Collaborator

aignas commented Mar 11, 2025

Let's merge this as this brings us to a better place than we are today.

@aignas aignas added this pull request to the merge queue Mar 11, 2025
Merged via the queue into bazel-contrib:main with commit 8f51731 Mar 11, 2025
4 checks passed
@aignas
Copy link
Collaborator

aignas commented Apr 10, 2025

@BurnzZ, if you have time, could you please take a look at #2762? I think failing when there are no python sources is a little bit troublesome.

jacky8hyf added a commit to jacky8hyf/rules_python that referenced this pull request Aug 13, 2025
With bootstrap_impl=system_python, after
bazel-contrib#2607,
host Python 3.6 and above is required because of the use of multiline
f-strings. For maximum backwards compatibility (because this is a host
dependency), do not use multiline f-strings.

This issue naturally does not apply to bootstrap_impl=script because
there are no host Python dependency.

Link: bazel-contrib#2607
jacky8hyf added a commit to jacky8hyf/rules_python that referenced this pull request Aug 13, 2025
With bootstrap_impl=system_python, after
bazel-contrib#2607,
host Python 3.6 and above is required because of the use of multiline
f-strings. For maximum backwards compatibility (because this is a host
dependency), do not use multiline f-strings.

This issue naturally does not apply to bootstrap_impl=script because
there are no host Python dependency.

Link: bazel-contrib#2607
github-merge-queue bot pushed a commit that referenced this pull request Aug 13, 2025
With bootstrap_impl=system_python, after
#2607, host Python 3.6
and above is required because of the use of multiline f-strings. For
maximum backwards compatibility (because this is a host dependency), do
not use multiline f-strings.

This issue naturally does not apply to bootstrap_impl=script because
there are no host Python dependency.

Link: #2607
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.

bazel coverage fails when using certain Python packages

4 participants