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

Code coverage baked into setup.cfg breaks Gentoo Portage builds/testing #5242

Closed
Kangie opened this issue May 13, 2024 · 8 comments · Fixed by #5312
Closed

Code coverage baked into setup.cfg breaks Gentoo Portage builds/testing #5242

Kangie opened this issue May 13, 2024 · 8 comments · Fixed by #5312
Assignees
Labels
testing Relates to the testing/CI infrastructure

Comments

@Kangie
Copy link

Kangie commented May 13, 2024

Problem

Code coverage baked into setup.cfg/pytest in 8b4983f
results in Gentoo's portage builds being unable to initialise tests as Gentoo pytest configuration disables code coverage due to irrelevance to our use case.

>>> Test phase: media-sound/beets-9999
python3.12 -m pytest -vv -ra -l -Wdefault -Werror::pytest.PytestUnhandledCoroutineWarning --color=yes -o console_output_style=count -o tmp_path_retention_count=0 -o tmp_path_retention_policy=failed -p no:cov -p no:flake8 -p no:flakes -p no:pylint -p no:markdown -p no:sugar -p no:xvfb -p no:pytest-describe -p no:plus -p no:tavern -p no:salt-factories
ERROR: usage: __main__.py [options] [file_or_dir] [file_or_dir] [...]
__main__.py: error: unrecognized arguments: --cov=beets --cov=beetsplug --cov-report=xml:.reports/coverage.xml --cov-report=html:.reports/html --cov-branch --cov-context=test
  inifile: /var/tmp/portage/media-sound/beets-9999/work/beets-9999/setup.cfg
  rootdir: /var/tmp/portage/media-sound/beets-9999/work/beets-9999

I can work around this by seding out the pytest options, but there's probably a better way to resolve this permanently.

sed -i -e "/--cov=beets/,+9d" setup.cfg

Setup

  • OS: Gentoo Linux
  • Python version: 3.12
  • beets version: master
  • Turning off plugins made problem go away (yes/no): N/A
@Kangie Kangie changed the title Code coverage baied into setup.cfg breaks Gentoo Portage builds/testing Code coverage baked into setup.cfg breaks Gentoo Portage builds/testing May 26, 2024
@bal-e
Copy link
Member

bal-e commented Jun 13, 2024

The latest master branch uses Poetry, where poe test --no-cov is apparently now a thing. Does this suitably disable coverage testing for the Gentoo case?

@Kangie
Copy link
Author

Kangie commented Jun 13, 2024

Just tested, I still need to run the sed in our ebuild to make this work, even if I manually suffix our test invocation with --no-cov. :(

I note that -p no:cov should accomplish the same thing and that's likely what's causing this issue to come up.

python3.12 -m pytest -vv -ra -l -Wdefault -Werror::pytest.PytestUnhandledCoroutineWarning --color=yes -o console_output_style=count -o tmp_path_retention_count=0 -o tmp_path_retention_policy=failed -p no:cov -p no:flake8 -p no:flakes -p no:pylint -p no:markdown -p no:sugar -p no:xvfb -p no:pytest-describe -p no:plus -p no:tavern -p no:salt-factories -p xdist -n 32 --dist=worksteal --deselect test/test_ui.py::CompletionTest::test_completion -n32 -v --no-cov
ERROR: usage: __main__.py [options] [file_or_dir] [file_or_dir] [...]
__main__.py: error: unrecognized arguments: --cov=beets --cov=beetsplug --cov-report=xml:.reports/coverage.xml --cov-report=html:.reports/html --cov-branch --cov-context=test --no-cov
  inifile: /var/tmp/portage/media-sound/beets-9999/work/beets-9999/setup.cfg
  rootdir: /var/tmp/portage/media-sound/beets-9999/work/beets-9999

@bal-e
Copy link
Member

bal-e commented Jun 13, 2024

I see. setup.cfg unconditionally adds the --cov arguments to the pytest command-line. pytest itself is invoked by poe from configuration in pyproject.toml, and reads from setup.cfg.

I can think of a few potential solutions here:

  1. pytest.ini should have a way to provide arguments specific to plugins. These arguments would be added by default, and the plugins they are for would be required, unless they were explicitly disabled like with -p no:cov. This will have to be suggested over at https://github.com/pytest-dev/pytest.

  2. The poe test task could be modified not to include coverage, and a new poe test-cov task could be added which does include coverage. The configuration for the latter would add all the coverage-related options. Running coverage makes poe test take noticeably longer, and in most cases I don't think it's necessary (developers just want to know that all the tests are passing), so this shouldn't cause too many problems, and may speed things up a bit.

  3. pytest could be invoked through a small wrapper script that only adds the necessary --cov options when --no-cov is not passed to it. This would make beets' testing setup a lot more opaque, IMO. It would be difficult to put those configuration options somewhere visible.

I think the best way forward is to implement (2) while filing an issue on pytest for (1).

@bal-e
Copy link
Member

bal-e commented Jun 15, 2024

@Serene-Arc can I ask for your opinion here? I want to make a PR which moves the current behavior of poe test to poe test-cov (IMO poe test shouldn't be doing coverage at all, it's usually unnecessary). This will allow testing to work without the pytest-cov plugin installed, which is what this issue is about.

@Serene-Arc
Copy link
Contributor

@snejus would be the one to talk to, he was just changing that part of the GH actions

@snejus snejus self-assigned this Jun 15, 2024
@snejus
Copy link
Member

snejus commented Jun 15, 2024

Hi @Kangie! Apologies that this broke Gentoo portage builds.

There's no reason to have the coverage running be default. I think I'd find more reasons why it should not run by default: it does not just break your builds, but it's also in the way of testing the project locally, since coverage calculation across such a large project adds a significant delay to tests.

I see. setup.cfg unconditionally adds the --cov arguments to the pytest command-line. pytest itself is invoked by poe from configuration in pyproject.toml, and reads from setup.cfg.

  1. The poe test task could be modified not to include coverage, and a new poe test-cov task could be added which does include coverage. The configuration for the latter would add all the coverage-related options. Running coverage makes poe test take noticeably longer, and in most cases I don't think it's necessary (developers just want to know that all the tests are passing), so this shouldn't cause too many problems, and may speed things up a bit.
    ...
    I think the best way forward is to implement (2) while filing an issue on pytest for (1).

@bal-e completely agree with you. I'm glad you're on the same page regarding the solution - this is exactly how I considered going about it.

Let me hack a quick PR with the above.

@snejus snejus added the testing Relates to the testing/CI infrastructure label Jun 15, 2024
@bal-e
Copy link
Member

bal-e commented Jun 15, 2024

Thanks @snejus! Ping me if you want a review on the PR (though I don't think it's necessary, the change should be a few lines at the most).

@snejus
Copy link
Member

snejus commented Jun 15, 2024

See #5312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Relates to the testing/CI infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants