Posted by webchick on November 17, 2009 at 6:33pm
7 followers
| Project: | Project issue file test |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | closed (duplicate) |
| Issue tags: | webchick's D7 alpha hit list |
Issue Summary
Or... something.
Issue: http://drupal.org/node/592018
http://drupal.org/node/592018#comment-2244466 shows as passing
Test shows as passing, but patch no longer applies as of today (http://drupal.org/node/592018#comment-2275696)
Its test results page at http://qa.drupal.org/pifr/test/16436 show that it was last tested on 11/11.
There was a re-test request issued on 11/13 http://drupal.org/node/592018#comment-2261652. Damien suspects that this may not have been received by the testing server, or may have otherwise gummed things up?
I confirmed that on d.o, the re-test interval is set to 1 day.
At any rate, something funny is going on here and has already caused me to break HEAD once and manually try and apply at least 3-4 non-applying patches.
Comments
#1
Hmm...should only be re-testing patches based on the interval that are in issues that still meet criteria and are the last patch on the issue.
Any forceful re-testing of a patch circumvents the interval. From looking at links, Damien's re-test request the log shows the patch was infact re-tested. I can create a test issue and play with this, but otherwise things seem ok.
#2
Another one: #634240: [HEAD BROKEN] system_list() caching is broken
#3
I will be checking things out at #636572: Testing the testing system TM.
#4
post patch
queued
sent
tested
result
retrieved
re-test
...
retrieved
http://qa.drupal.org/pifr/test/19374
All worked like a charm, so either we had some flukes?
I am going to play with it a bit more, but it seems to work fine.
#5
Here's another one:
http://drupal.org/node/502190#comment-2253612
This showed passing green and last tested 11/11. It wasn't until jhodgdon requested a re-test manually that it came back as failing, even though I'm sure it fell out of sync with HEAD days ago. Unfortunately, when you click "Test results" you get to http://qa.drupal.org/pifr/test/19472 which shows no trace of the 11/11 test.
Is this smelling more like a problem with Drupal.org's cron not properly passing patches along? The initial test always seems to work fine, but then there seems to be no follow-up runs done after that.
#6
Though it also seems like it ought not to be obliterating the old test results, come to think of it.
#7
Yet another: http://drupal.org/node/221081#comment-2274752
Last tested 11/17/2009 13:37:57: http://qa.drupal.org/pifr/test/19246. Test count is only 1. Today is 11/20/2009 20:09:xx.
I'm going to go out on a limb here and say that patches are never being automatically retested anymore.
Is this enough info to go on? Can I mark this as active?
#8
FWIW, it looks like the automatic re-testing stopped around 12 November:
http://qa.drupal.org/pifr/test/13855
http://qa.drupal.org/pifr/test/15701
http://qa.drupal.org/pifr/test/16169
#9
The issue is in the API filter: the query in pift_cron_retest() tries to match a core API version (ie '6', '7') against a term id (ie. 103 for '7.x'). I think we need the following patch as a very minimum.
#10
But yet?
- if (!in_array($api_version, array_filter(variable_get('pift_core', array())))) {+ if (!in_array($api_tid, array_filter(variable_get('pift_core', array())))) {
It attempted to compare version against version. I'll look at this patch in more detail as it may have other areas it touches, but I do not see a flaw unless $api_version isn't be determined properly, but that is a different bug.
#11
Look more closely, the issue is in pift_cron_retest() trying to compare a version number with a term ID. It makes more sense to directly store term IDs instead of version number, hence the change you pin-poined.
#12
I would love to get this resolved by the first alpha. Tagging.
#13
Waiting on #669322: Update the scratch environments for drupal.org so I can get some serious testing in and find out where this is. From manual testing and such it would seem to be a PIFT issue somewhere in the cron that runs and sends all patches that match criteria to qa.d.o. I highly highly doubt it has anything to do with qa.d.o receiving them as the system doesn't differentiate the requests at that level.
The query was changed for performance reasons (mysql hates OR) (right after deployment) which changed the logic rather significantly. My guess is that is has something to do with that as I did rather extensive testing of the feature before deployment.
#14
#15
I have rewritten all the related code to make a bit more sense and I believe solve this problem.
The area that causes this problem is:
<?php"ON (n.vid = t.vid AND t.tid IN (" . db_placeholders($api_versions, 'int') . "))"
?>
Since $api_versions before was 6, 7, 8 instead of the term IDs. In all other locations the code worked as expected and in some places it needs the 6, 7, 8 which is why the code existed. The majority of checks can and are better of using the tid so I have changed them as such.
#16
Updated after some testing.
#17
Committed.
#18
That looks like the correct fix. I'm not sure refactoring was really needed nor wise (my patch was the minimum fix for that particular issue). This new code probably do not work as expected:
<?php+ // Ensure that one of the compatibility terms is present on the release node.
+ $release = node_load($node->project_issue['rid']);
+ if (!in_array(pift_core_api_versions(), array_keys($release->taxonomy)) {
return FALSE;
}
?>
#19
in_array() works with array as needle, but I was wrong in thinking that it would return true if one was found...it requires all. I have already fixed it as you can see in the second patch from #16 after testing.
#20
#21
Automatically closed -- issue fixed for 2 weeks with no activity.
#22
But of course since this doesn't work, reopening.
#23
Marking as duplicate of #675460: Ensure/Refactor cron_retest() query to only re-test the last file on an issue; Automatically retest RTBC patches. Added a a link here to that issue, so that folks can reference the historical work, and an understanding of how the current code is 'supposed' to work.