#909 Suggest that linting code and measuring coverage is not %check's business
Opened 5 months ago by churchyard. Modified 2 months ago

See https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/6FLC4MJREITVXJUAK4CQCJ462FGL7WGS/ cc @vondruch @pviktori

Would it be generally acceptable by the FPC if we recommend to disable code linting and test coverage metrics in %check?

In my opinion, linting code (such as with pylint, flake8 or pycodestyle in the Python world) or measuring test coverage is upstream's business and not downstream's. Aka if the test coverage is only 99%, it should not abort the build and if the upstream recommendation about line length changes, it should not abort the build either.

Practically, this also means that linting tools should stop being essential buildrequires for critical components (e.g. again in Python world, broken pylint on Python 3.8 should not prevent us to bootstrap packages up to a functional compose).


+1 to this.

In Ruby world, people tend to include SimpleCove, Coveralls or Rubocop for the same purpose. But it just increases the dependency chain and build times.

Yes, although I would appreciate if there would be some automated ways to fix that. To give more background, I have had some packages which do some coverage testing and to drop that I had to change tox.ini/setup.cfg or something like that.

Actually, I would like to discourage usage of the specific packages and actively remove them from the build dependencies.

Yes, although I would appreciate if there would be some automated ways to fix that.

This [1] was my proposal for Ruby packages.

FWIW, I'm generally +1 to the concept here, but I think the recommendation should not be too strong. If upstream makes running a linter optional, by all means we should recommend that the packager not do it; but I think recommending or requiring that lint / coverage checks be patched out if they're wired into the test process by upstream might be going too far. Instead of that we could suggest that if a packager runs into this scenario, they have the package run the linter unless that's not possible (in which case it must be patched out), but consider sending a patch upstream to make it optional if they have the skills to do that.

Metadata Update from @james:
- Issue tagged with: meeting

5 months ago

I don't agree that we should recommend that things be disabled in %check, but I do think it's reasonable to explicitly allow it and give a couple of useful reasons why the maintainer might want to do such a thing.

One problem I do have with a wholesale disablement of such tests is that they do potentially give useful output in the case that Fedora is patching the upstream source. Even something as pointless to Fedora as a documentation spellcheck can still be useful if the documentation is being patches.

In any case, currently the only text we have in the Test Suites section is:

If the source code of the package provides a test suite, it should be executed in the %check section, whenever it is practical to do so.

So how about:

If the source code of the package provides a test suite, it SHOULD be executed in the %check section whenever it is practical to do so. Individual tests MAY be disabled when necessary, but the reasons for disabling tests MUST be documented in the spec. Valid reasons for disabling some tests include expected failures, tests which require extensive additional dependencies, or those which provide little or no value to Fedora (such as code linting or coverage tests, or spell checking of documentation).

I don't agree that we should recommend that things be disabled in %check, but I do think it's reasonable to explicitly allow it and give a couple of useful reasons why the maintainer might want to do such a thing.

One of them might be running exactly those tests in Fedora CI.

If the source code of the package provides a test suite, it SHOULD be executed in the %check section whenever it is practical to do so. Individual tests MAY be disabled when necessary, but the reasons for disabling tests MUST be documented in the spec. Valid reasons for disabling some tests include expected failures, tests which require extensive additional dependencies, or those which provide little or no value to Fedora (such as code linting or coverage tests, or spell checking of documentation).

+1 to this.

An advantage of running linting/coverage tools in a wide variety of packages is that it helps to find issues in those tools themselves, when for instance there are changes in their dependencies. I have experience of this in the perl world, and koschei finds some of these issues quite quickly, which can be useful for getting timely fixes done upstream.

An advantage of running linting/coverage tools in a wide variety of packages is that it helps to find issues in those tools themselves, when for instance there are changes in their dependencies. I have experience of this in the perl world, and koschei finds some of these issues quite quickly, which can be useful for getting timely fixes done upstream.

This implies that 1) koschei is turned on for all packages 2) maintainers actually react when koschei rebuild fails. I think none of this true for most of the packages. That means, mass rebuild will fail on many packages due to this.

If the source code of the package provides a test suite, it SHOULD be executed in the %check section whenever it is practical to do so. Individual tests MAY be disabled when necessary, but the reasons for disabling tests MUST be documented in the spec. Valid reasons for disabling some tests include expected failures, tests which require extensive additional dependencies, or those which provide little or no value to Fedora (such as code linting or coverage tests, or spell checking of documentation).

I can +1 this as a compromise.

For the record, here's my +1.

If the source code of the package provides a test suite, it SHOULD be executed in the %check section whenever it is practical to do so. Individual tests MAY be disabled when necessary, but the reasons for disabling tests MUST be documented in the spec. Valid reasons for disabling some tests include expected failures, tests which require extensive additional dependencies, or those which provide little or no value to Fedora (such as code linting or coverage tests, or spell checking of documentation).

This proposal expands a bit on the current "test suites" guidelines, that is fine. However, in the context of "linters" and "code coverage", it makes little sense. While the last remark tries to cover this ticket, the guidelines would be probably better without it.

From this POV, I'd be probably better if there is no modification to this section or if we could be more explicit about code coverage and linters. While I see limited benefit in linters (but if there are modification applied on top of original code, such modification should be upstreamed and covered by upstream tests), I really see no benefit in code coverage.

Login to comment on this ticket.

Metadata