search

ansible / ansibullbot

Bot for management of Ansible issues and PRs on GitHub.
GNU General Public License v3.0
203 stars 126 forks source link

check coverage for backports #1020

Open jctanner opened 6 years ago

jctanner commented 6 years ago

On any backport, check:

1) it was cherry picked 2) the thing has a test target 3) the code changed has coverage 4) it's not a new module/plugin/feature/etc

If either fails, it's an "invalid" backport.

jctanner commented 6 years ago

@mattclay @gundalow thoughts?

mattclay commented 6 years ago

The first item is related to this feature request: https://github.com/ansible/ansibullbot/issues/1016

mattclay commented 6 years ago
  1. the thing has a test target

Probably not necessary, assuming the next item is implemented. Not everything that has coverage has a dedicated test target.

  1. the code changed has coverage

We'll either need to run the tests with coverage enabled, or consult codecov.io for the latest data from devel, which unfortunately won't always be representative of the test coverage in the stable branch receiving the backport for several reasons:

  1. the backport itself may include tests which haven't run in the nightly coverage run yet
  2. tests were added and run from devel that are not present in the stable branch or the backport
  1. it's not a new module/plugin/feature/etc

We could check for a few things:

  1. new module files (easy)
  2. new plugin files (easy)
  3. new module parameters

That last one (item 3) is probably best done as part of validate-modules. It already checks for new module parameters and looks for the version added, so we could leverage that code and make additions to stable branches report an error for new parameters.

gundalow commented 6 years ago

I'd just check for

it was cherry picked the thing has a test target the code changed has coverage it's not a new module/plugin/feature/etc

I think that's an improvement over what we have now without a huge amount of bit work. This can always be expanded in the future.

@mattclay does the above sounds OK? You spend a lot more time than me looking at backport PRs.

We may need to check that validate-modules is testing for this.

mattclay commented 6 years ago

@gundalow Yes, I think we're both suggesting basically the same thing.