CI: new check for leftover skips/fixmes #15096

edsantiago · 2022-07-28T01:07:15Z

If a PR says "Fixes #123", make sure it removes skips and/or
FIXME comments that reference issue 123.

Signed-off-by: Ed Santiago [email protected]

Internal (CI): better cross-checks between PR "Fixes" messages and skips/comments in code

openshift-ci · 2022-07-28T01:07:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [edsantiago]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2022-07-28T01:09:07Z

Just something I started on Monday then lost track of. I just did some polishing and am submitting as a short-n-sweet possibility for our CI. Runs in near-zero time. Sample output:

pr-removes-fixed-skips: Your PR claims to resolve issues #15025, #123
    ...but does not remove associated Skips/FIXMEs:

   test/system/030-run.bats:73:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"
   test/system/030-run.bats:809:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"
   test/system/170-run-userns.bats:33:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"
   test/system/170-run-userns.bats:41:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"
   test/system/400-unprivileged-access.bats:10:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"
   test/system/500-networking.bats:748:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"
   test/system/500-networking.bats:87:    skip_if_cgroupsv1 "FIXME: #15025: run --uidmap fails on cgroups v1"

Please do not leave Skips or FIXMEs for closed issues.

If an issue is truly fixed, please remove all Skips referencing it.

If an issue is only PARTIALLY fixed, please file a new issue for the
remaining problem, and update remaining Skips to point to that issue.

And if the issue is fixed but the Skip needs to remain for other
reasons, again, please update the Skip message accordingly.

Luap99 · 2022-07-28T07:50:27Z

contrib/cirrus/pr-removes-fixed-skips

+##################
+sub fixed_issues {
+    # gitHUB commit message, not the git one.
+    my $change_message = $ENV{CIRRUS_CHANGE_MESSAGE}


Does it only check the PR description then? I think it would also make sense to check for the actual commit messages. From past experiences the issues are closed when you merged commits with Fixes #XXXX. It doesn't have to be in the PR description.

Checking commit messages is harder: there's a lot of git involved. I was going for good-enough, not 100%. When time allows, I'll see what it takes to check git.

I thought you already check commits for [NO NEW TESTS NEEDED]?
Either way using the PR descriptions is already much better than nothing. This is definitely not a blocker for me.

Luap99 · 2022-07-28T07:59:50Z

contrib/cirrus/pr-removes-fixed-skips

+        'verbose|v'  => \$verbose,
+
+        help         => \&usage,
+        man          => \&man,


This is not used?

Luap99 · 2022-07-28T08:04:08Z

contrib/cirrus/pr-removes-fixed-skips

+        'dry-run|n!' => sub { $NOT = ' [NOT]' },
+        'force'      => \$force,
+        'verbose|v'  => \$verbose,


$NOT, $force and $verbose are not used? I don't know perl so maybe I am missing something?

Good eyes. I use a script template that I've honed over years, with all the usually-needed crap that I then trim down. I missed trimming-down some stuff. Will clean up - thanks.

contrib/cirrus/pr-removes-fixed-skips

Luap99 · 2022-07-28T08:18:14Z

contrib/cirrus/pr-removes-fixed-skips.t

+== no match if Skip is commented out
+[123]
+! test/e2e/foo_test.go:10:   // Skip("#123: commented out")
+! test/system/012-foo.bats:20:      # skip "#123: commented out"


This does not seem to work correctly for all cases. This test will fail:

! test/e2e/foo_test.go:10: // Skip("FIXME: #123 commented out") ! test/system/012-foo.bats:20: # skip "FIXME: #123 commented out"

But do we actually want to exclude commented skips? Skips should be removed and not commented out, especially when the issues is supposedly fixed.

Again, I was going for quick-easy. To understand the problem with comments we have to go back to the grep expression in line 129 above. I found it safest to check for:

skip with only leading whitespace

FIXME anywhere on the line

I can't find any instances right now like '// make sure we don't skip bytes (#11496)` in the code itself, only in release notes, but I could imagine someone adding that and getting very upset about being blocked for it. Unlikely, I know.

Again, this is not meant to be perfect. I do manual scans of the repo every once in a while, with a much looser expression, because I don't mind manually double-checking the questionable cases. The purpose of this is simply to make my job a little easier, so I can spend my days sipping margaritas on the beach.

rhatdan · 2022-07-28T12:22:08Z

I think we can enhance this going forward
LGTM

If a PR says "Fixes #123", make sure it removes skips and/or FIXME comments that reference issue 123. Signed-off-by: Ed Santiago <[email protected]>

Luap99 · 2022-07-28T15:22:49Z

/lgtm
/hold

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jul 28, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2022

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 28, 2022

edsantiago force-pushed the skips_are_removed branch from c0905a4 to 7723c0f Compare July 28, 2022 01:18

Luap99 reviewed Jul 28, 2022

View reviewed changes

CI: new check for leftover skips/fixmes

6764fe0

If a PR says "Fixes #123", make sure it removes skips and/or FIXME comments that reference issue 123. Signed-off-by: Ed Santiago <[email protected]>

edsantiago force-pushed the skips_are_removed branch from 7723c0f to 6764fe0 Compare July 28, 2022 15:00

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2022

openshift-ci bot assigned Luap99 Jul 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2022

edsantiago removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2022

openshift-merge-robot merged commit 5eb06e7 into containers:main Jul 28, 2022

edsantiago deleted the skips_are_removed branch July 28, 2022 18:08

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: new check for leftover skips/fixmes #15096

CI: new check for leftover skips/fixmes #15096

edsantiago commented Jul 28, 2022 •

edited

Loading

openshift-ci bot commented Jul 28, 2022

edsantiago commented Jul 28, 2022

Luap99 Jul 28, 2022

edsantiago Jul 28, 2022

Luap99 Jul 28, 2022

Luap99 Jul 28, 2022

Luap99 Jul 28, 2022

edsantiago Jul 28, 2022

Luap99 Jul 28, 2022

edsantiago Jul 28, 2022

rhatdan commented Jul 28, 2022

Luap99 commented Jul 28, 2022

CI: new check for leftover skips/fixmes #15096

CI: new check for leftover skips/fixmes #15096

Conversation

edsantiago commented Jul 28, 2022 • edited Loading

openshift-ci bot commented Jul 28, 2022

edsantiago commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jul 28, 2022

Luap99 commented Jul 28, 2022

edsantiago commented Jul 28, 2022 •

edited

Loading