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

Cleanup some test and coverage configuration #381

Closed
wants to merge 8 commits into from

Conversation

cidrblock
Copy link
Collaborator

  • Add tox, pytest and pytest-xdist to the test deps
  • Update the coverage report configuraion in the pyproject.toml
  • Switch the coverage source to the package name for easier troubleshooting when running `coverage run --debug trace"
  • Remove all coverage files from the tox venv (.coverage and .coverage.xxx)
  • Enable xdist and add necessary .pth file for xdist forks to start coverage collection

tox.ini Outdated
@@ -43,16 +43,21 @@ extras =
commands_pre =
# safety measure to assure we do not accidentally run tests with broken dependencies
{envpython} -m pip check
# Add the pth file for coverage + xdist reporting
python -c 'import pathlib; pathlib.Path("{env_site_packages_dir}/cov.pth").write_text("import coverage; coverage.process_startup()")'
Copy link
Member

Choose a reason for hiding this comment

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

@cidrblock I would really want to avoid this hack. What we can do to avoid it? -- i would rather use a plugin instead of it.

Copy link
Collaborator Author

@cidrblock cidrblock May 22, 2024

Choose a reason for hiding this comment

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

I'm not sure there is anyway to avoid it, I guess we could "hide" it, although I think up front here it's a little more obvious what's actually going on.

There exists the coverage-enable-subprocess python package which has no README, license and only 10 stars which I see you commented on today, but does the exact same thing.

Another implementation of the same exists here, but again not a popular project, and does the exact same thing.

Having a look at sourcegraph, it appears some people do the same, except in their github action, which would be really difficult to test locally if there was an issue.

The approach here is exactly what is suggested in the docs for coverage:

Create a .pth file in your Python installation containing:

import coverage; coverage.process_startup()

https://coverage.readthedocs.io/en/latest/subprocess.html

I don't have feeling here one way or another, but it is nice to finally know how this exactly works for coverage which should allow us to get coverage for subprocess in tests easier as long as the same interpreter is used and the env var is carried through.

Because it is interesting, pip has a pytest venv fixture where they write the file out to the new venv. I suspect we may need something like this at some point if we're running a test in a fresh venv

I'm open to suggestions, but based on the time I've spent on this, the tox approach seems like the most obvious and straight forward approach given what needs to happen for a new proc to write coverage with the config file we have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I neglected to mention the pytest-cov project, which again is doing the same thing: https://github.com/pytest-dev/pytest-cov/blob/5295ce01c84262cec88f31255e9ac538718f3047/setup.py#L27 but seems like a heavy dep to bring in for just the pth file injection which will work fine if tox is the entrypoint.

(and because of the work you did with the combining of the coverage files from the tox venv and building the lcov file in the root, we'll get the feedback we need within VsCode)

@cidrblock
Copy link
Collaborator Author

This is out of date based on the better config in ADE and ADT, will revise at some point in the future

@cidrblock cidrblock closed this May 27, 2024
auto-merge was automatically disabled May 27, 2024 14:18

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants