Problem/Motivation

Testbot currently ignores patches with a -D#.patch filename ending. The intent of this is to prevent testbot from testing patches for a different core version. This convention is confusing:

  • Many expect that naming a patch patchname-D7.patch will automatically cause that patch to be tested against D7, but this does not happen.
  • Even if an issue is filed against 8.x, a patch with the filename patchname-d8.patch will not be tested.

Proposed resolution

  • To have testbot ignore a patch, name it patchname-do-not-test.patch.
  • The -D# suffixes will no longer cause testbot to ignore a patch.

Original report by pwolanin

I'm told right now that not only are patches suffixed with -D6 ignored for core testing, but also -D7 and -D8. That makes it very hard to provide a uniform set of instructions for naming patches, and is rather confusing.

More broadly, it would be nice to have some way to flag each patch with not just a core version but a project branch that it's relevant to, since it's often the case that one want's to upload a patch and its backport at the same time (e.g. between 2 branches that work with the same core version).

Comments

pwolanin’s picture

Version: » 6.x-2.x-dev

probably really a 6.x-2.x issue if that's what's deployed.

rfay’s picture

Version: 6.x-2.x-dev »

6.x-3.x is what's deployed.

I'd rather have a naming scheme that explicitly says -testD6only or something like that. Would something like that work?

In general, I'd rather not even have this be more than a footnote in the patching instructions, as it's a very advanced use case.

boombatower’s picture

Version: » 6.x-2.x-dev

If we are trying to move forward with the merged code considering how small the git changes are and that 6.x-2.x has been merged lets solve any issues there?

webchick’s picture

Throwing my support behind this issue. When I spend time with new (and even more experienced) contributors, this comes up ALL the time. And each time the explanation takes about 15 minutes and still leaves them glassy-eyed by the end. :\

What happens is people see other people they know know what theyr'e doing upload patches with -D7, -D8 extensions. They naturally assume this is a naming convention to trigger the testbot to test against those versions, so they also do the same thing. Hilarity ensues:

Eager New Contributor: "Hey! How come testbot's ignoring my patch?"
Wizened Old Contributor: "Oh, you have a -D8 extension on it. You need to remove that."
Eager New Contributor: "Ok, done. Wait. Why did it mark my test as a fail?"
Wizened Old Contributor: "Oh, because you also uploaded a D7 version of your patch. THAT one needs a -D7 extension, so that testbot ignores it."
Eager New Contributor: "OK..... Well, yay! My D8 patch got committed. Now I moved the issue down to D7. How come testbot's still ignoring my patch?"
Wizened Old Contributor: "Oh. Because you need to re-upload it *again*, but this time without the D7 extension."
Eager New Contributor: "Wow. I think I'm going to go outside and give up on this core contributing thing. It's complicated."

So... anything we can do to make this easier on people would be great. :)

jthorson’s picture

The 'ignore -Dx' piece is implemented via the file pattern regex ... see the 'Criteria' fieldset at /admin/project/pift.

rfay’s picture

So this is just a configuration element... We could change it to something more rational. Now the question is "What's rational".

I'd like to have the opposite of what we have (please *do* test this on D7, instead of please don't test this on D7). I don't think we have that capability.

But the very least we could do is to change the regex to "-exclude-from-d7.patch". Hiding this information in the patch name is the problem in the first place of course. We should have something better than that. But what about changing the regex to "exclude-from_d[6789]" ?

webchick’s picture

Well, I think if this regex just governs what patches the testbot should ignore, the obvious extension to me would be just "-ignore". :) D6 vs. D7 will be indicated in the patch name somehow else for humans.

jthorson’s picture

+1 for -ignore ... the 'test this against D7/8/9' functionality is possible once we get the 'nid' field added; I started down that road in a sandbox a while back.

rfay’s picture

Status: Active » Needs review

Proposal then:

Change "Criteria" at admin/project/pift from
/(?<![.\-_][Dd]\d)(\.diff|\.patch)$/
to
/(?<![.\-_]ignore[-_]*[Dd]\d)(\.diff|\.patch)$/

Change the File description section from:

Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that apply specifically to a version of core other than the one the issue is pointing to, you can prefix the extension with <em>-D(number)</em> to prevent them from being queued for testing. For example, <em>foo.patch</em> and <em>bar.diff</em> would both be queued for testing, whereas <em>foo-D5.patch</em> and <em>bar-D6.diff</em> would not be queued for testing.

to

Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that apply specifically to a version of core other than the one the issue is pointing to, you can prefix the extension with <em>-ignore-D(number)</em> to prevent them from being queued for testing. For example, <em>foo.patch</em> and <em>bar.diff</em> would both be queued for testing, whereas <em>foo-ignore-D6.patch</em> and <em>bar-ignore-D7.diff</em> would not be queued for testing on D6 and D7 respectively.
webchick’s picture

No, I want to change it to just:

/(?<![.\-_]ignore)(\.diff|\.patch)$/

Want testbot to ignore your patch? Put -ignore at the end. :) Done.

rfay’s picture

Status: Needs review » Needs work

I'd be ok with that. It's simple. However, it's a bit of mucking about when an issue changes version. You have to download the patch and change its name. I think that's OK. I'd rather not go looking up in the issue for what version a patch is built for.

Since PIFT explicitly pulls out the core version from the patch and processes using it, we'd have to look at the code to see if that works. Not as easy, perhaps.

rfay’s picture

Status: Needs work » Needs review

I think "ignore" is pretty ambiguous. Suggesting do_not_test.

Proposal:

Change "Criteria" at admin/project/pift from
/(?<![.\-_][Dd]\d)(\.diff|\.patch)$/
to
/(?<![.\-_]do[-_]not[-_]test)(\.diff|\.patch)$/

Change the File description section from:

Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that apply specifically to a version of core other than the one the issue is pointing to, you can prefix the extension with <em>-D(number)</em> to prevent them from being queued for testing. For example, <em>foo.patch</em> and <em>bar.diff</em> would both be queued for testing, whereas <em>foo-D5.patch</em> and <em>bar-D6.diff</em> would not be queued for testing.

to
Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that should not be tested by the testbot (for example, patches that do not work with the current API version in the issue), add "-do_not_test" to the patch. For example "drupal.do_nothing_555555-do-not-test.patch"

jthorson’s picture

For ease of typing (and sacrificing some intuitiveness), perhaps '-dnt' for 'do-not-test'?

webchick’s picture

I prefer legible over shorthand, personally. I can't begin to overstate how utterly perplexing the testbot is to new contributors. :( I really don't want to add any more weird things we need to document.

#12 looks good to me. We should probably run this by a couple of newbies too.

jthorson’s picture

Makes sense.

Now ... what if we made it 'do-not-test' anywhere in the filename, to allow for 'do-not-test-D7.patch' patterns?

webchick’s picture

Even better.

jthorson’s picture

Category: bug » task

This is just a configuration item ... we should make it happen. :)

rfay’s picture

Status: Needs review » Reviewed & tested by the community

OK, I'm going to propose and RTBC at the same time, based on the comments above. This is a slight editing of #12. If no objections are noted, jthorson or I will just deploy this.

Proposal:

Change "Criteria" at admin/project/pift from
/(?<![.\-_][Dd]\d)(\.diff|\.patch)$/
to
/(?<!do[-_]not[-_]test).*(\.diff|\.patch)$/

Change the File description section from:

Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that apply specifically to a version of core other than the one the issue is pointing to, you can prefix the extension with <em>-D(number)</em> to prevent them from being queued for testing. For example, <em>foo.patch</em> and <em>bar.diff</em> would both be queued for testing, whereas <em>foo-D5.patch</em> and <em>bar-D6.diff</em> would not be queued for testing.

to
Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that should not be tested by the testbot (for example, patches that do not work with the current API version in the issue), add "do-not-test" anywhere in the patch name. For example "drupal.do_nothing_555555-do-not-test.patch"

webchick’s picture

Sounds great!! Thanks, Randy! :D

rfay’s picture

OK, I'm up for doing this tonight. Just requires the change, looking for the page that documents this (if there is one) to edit it, testing the change, and publicizing it.

rfay’s picture

Sorry, I was only able to do #12 because of some strangeness with look-behind regexes or something. Or just a brain fart.

I used /(?<!do[-_]not[-_]test)\.(diff|patch)$/

So it's now

whateveryouwant-do-not-test.patch
whateveryouwantdo_not_test.patch

etc.

rfay’s picture

Status: Reviewed & tested by the community » Fixed

And of course I don't mind somebody pointing out what was wrong with my other regex.

xjm’s picture

Updated the summary because people are going to be confused about the change. :)
E.g. #1446372-30: Invalid Unicode code range in PREG_CLASS_UNICODE_WORD_BOUNDARY fails with PCRE 8.30

So we need to promote/publicize this somehow.

xjm’s picture

xjm’s picture

I'll make a tweet from @drupalcore later if folks don't have objections:

The suffix to make testbots ignore a #drupal issue queue patch has changed. Use name-do-not-test.patch. https://drupal.org/node/1092232

I don't think it really needs to go on groups/planet (which would autotweet it), but I'll check with webchick or chx first.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Issue summary: View changes

Removing empty