Testing of patches on Mollom's DRUPAL-6--1 branch leads to very odd test results in almost 50% of all test requests currently:

PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).

(example: http://qa.drupal.org/pifr/test/76304, but also many others)

When this happens, the patch is reported as "passed", however, with the addition of "zero passes", you do not know whether tests were executed at all or not.

Upon sending the identical patch for re-testing, it most often comes back with proper testing results (i.e., non-zero passes). Interestingly, even the plain branch test reported zero passes once, but similar to patch test results, the branch test result corrected itself after the next branch self-test (i.e., after the next commit).

Note that I've also encountered such odd test results with Drupal core patches against D7 (HEAD).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Priority: Normal » Major

It just happened again (over in http://drupal.org/node/872646#comment-3287492), but this time, I know that the patch should not have passed.

Hence, it looks like the result message is technically correct. There are actually no tests executed.

As maintainers may not recognize the "0" but just the green test status, this is a major bug, at minimum.

sun’s picture

Is this the wrong project/queue?

sun’s picture

The frequency in which this problem occurs increased, http://drupal.org/node/801662#comment-3299862

Only one of those re-tests led to a proper test result. All others returned with Previous result: PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).

sun’s picture

This time, getting

Previous result: FAILED: [[SimpleTest]]: [MySQL] 85 pass(es), 23 fail(s), and 0 exception(es).

from test client #33. We are missing ~1.500 assertions. Test results seem to indicate 1 unknown assertion that's interpreted as a pass:

Test name	Pass	Fail	Exception
Access checking (MollomAccessTestCase) [Mollom]	1	0	0
Blacklisting (MollomBlacklistTestCase) [Mollom]	1	0	0
Comment form protection (MollomCommentFormTestCase) [Mollom]	1	0	0
Contact form protection (MollomContactFormTestCase) [Mollom]	1	0	0
Data processing (MollomDataTestCase) [Mollom]	1	0	0
Fallback behavior (MollomFallbackTestCase) [Mollom]	1	0	0
Form administration (MollomFormConfigurationTestCase) [Mollom]	1	0	0
ExpandInstallation and key handling (MollomInstallationTestCase) [Mollom]	72	23	0

Message	Group	Filename	Line	Function	Status
Post installation warning found.	Other	mollom.test	708	MollomInstallationTestCase->testInstallationProcess()	
...
"Mollom testing mode is still enabled." found	Other	mollom.test	787	MollomInstallationTestCase->testInstallationProcess()	
Call to undefined function _mollom_status()	PHP Fatal error	mollom.install	26	Unknown	

Language detection (MollomLanguageDetectionTestCase) [Mollom]	1	0	0
Reporting functionality (MollomReportTestCase) [Mollom]	1	0	0
Reseller functionality (MollomResellerTestCase) [Mollom]	1	0	0
Server list recovery (MollomServerListRecoveryTestCase) [Mollom]	1	0	0
Server responses (MollomResponseTestCase) [Mollom]	1	0	0
User registration and password protection (MollomUserFormsTestCase) [Mollom]	1	0	0
boombatower’s picture

Project: Project Issue File Review » Project issue file test

This appears to be related to #102102: Parse project .info files: present module list and dependency information and the way PIFT instructs pifr to run tests..which is a know contrib testing bug...they will all be fixed together with .info parsing and updated in one shot.

dww’s picture

Yeah, this is also nailing me in the project queue, for example:
#69556-51: move "cvs access" tab into project, make it generic
The tests in that patch contain PHP errors if you run them locally. However, the bot said it ran no tests and that everything is happy and green. ;)

What's weird is that this behavior is intermittent. For example, the bot properly ran all the tests here:
#69556-31: move "cvs access" tab into project, make it generic
#69556-33: move "cvs access" tab into project, make it generic
...

sun’s picture

Is there anything I could do to help resolve this? It starts to get really annoying -- it almost looks like it happens for /any/ patch sent for a certain branch now. I.e., almost no issues with HEAD, but in 99% of all cases on DRUPAL-6--1

I'd go nuts if I wasn't able to invoke a re-test directly on http://qa.drupal.org/pifr/test/85109 - fortunately, I can, but even doing that directly takes hours for a single patch.

boombatower’s picture

#5 needs completion. All just renaming with like one function addition.

jhodgdon’s picture

http://qa.drupal.org/pifr/test/158364 (testing from #1069582-19: How to use an alternate stream wrapper when saving media files?) is also doing this.

In looking at the test log, I see:

Tests to be run:

Test run started: Monday, July 18, 2011 - 20:52

Test summary:
-------------

  ERROR: No valid tests were specified.].
[13:52:19] Command [/usr/bin/php ./scripts/run-tests.sh --php /usr/bin/php --url 'http://drupaltestbot664-mysql/checkout' --list 2>&1] succeeded
  Duration: 0 seconds
  Directory: [/var/lib/drupaltestbot/sites/default/files/checkout]
  Completion status: [0]
  Output: [Available test groups & classes

No valid tests were specified??? Not sure what it means, but that is one possible bit of information that can cause this 0 passes thing. It seems like if no tests are specified, that should count as a fail?

jthorson’s picture

The main issue in #9 can be seen a few rows up:

[13:59:20] Command [/usr/bin/php ./scripts/run-tests.sh --concurrency 8 --php /usr/bin/php --url 'http://drupaltestbot664-mysql/checkout' --file  2>&1] succeeded

The '--file' parameter expects an argument, containing the tests to run (defined by file) ... with no file argument, there is no test.

In trying to troubleshoot further, however, I keep getting the argument '--all' instead of '--file' ... so I can't explain 'why' the argument is missing (though from boombatower's comment in #5, it sounds dependency related).

rfay’s picture

Marked #1156976: 0 tests passing is not green as a duplicate. There's also good information over there.

Gábor Hojtsy’s picture

Happens for core tests too: http://qa.drupal.org/pifr/test/183394 Usually I've seen it coupled with other failures. Like first its says it cannot checkout from git. Then you ask it to retest, then it gets back with this 0 passes... :|

rfay’s picture

Project: Project issue file test » Project Issue File Review

I saw this today with http://qa.drupal.org/pifr/test/183394 - what happened was that the test ruined the testbot (used up the available tmpfs space for the database). The test returned a "0 tests passed" and the testbot never checked again, as mysql had died.

So a probable case for at least some of these is mysql dies (mysql server has gone away) and PIFR misinterprets something about that. We *should* be able to simulate this by just stopping mysqld.

Moving this to the PIFR queue, as this example is certainly PIFR.

Edit: I see this is the exact same test Gabor reported. Hmm. #1260586-12: Consolidate .po file import to one directory. Looks to me like if we just cease to test patches that Gabor posts, we'll be OK. I will try to see if every fill-up of /tmpfs is associated with a particular test.

rfay’s picture

I see that http://qa.drupal.org/pifr/test/183854, which is #1260586: Consolidate .po file import to one directory did this again. Seems to have broken all the testbots?

jthorson’s picture

So the biggest challenge to this issue is that the ability to determine whether '0 tests passed' is a legitimate response lies within PIFR, while the actual 'No tests found' error message is coming from run-tests.sh. As far as I know, we treat any non-zero response from run-tests.sh as a failure.

The approach in the patch at #1233248: Add check for empty --file argument before running run-tests.sh during simpletest reviews (which should probably be marked as a dupe) was to determine whether there are any actual tests to be run, and if so, execute run-tests.sh ... if not, return the 'no tests found' failure message without actually executing run-tests.sh. The only problem was with the assumption that no tests was a failure ... what we may need to do is have this scenario skip run-tests.sh but still return a 'pass' result, assuming the previous stages had all passed successfully. Alternatively, we can set a flag when the 'no test files found' condition is detected, execute run-tests.sh, and then analyze the run-tests.sh output; overriding the 'no tests found' failure condition and message if no tests were actually expected.

jthorson’s picture

Status: Active » Needs review
jthorson’s picture

Status: Needs review » Postponed

Patch referred to in #16 has been committed to 6.x-2.x-dev ... will postpone this for now, and revisit after the next release is deployed to the production testbots.

jthorson’s picture

Status: Postponed » Fixed

Seems to be resolved.

Status: Fixed » Closed (fixed)

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

jthorson’s picture

Status: Closed (fixed) » Active

Could use another revisit ... the file scan isn't the only scenario which can cause this.

sun’s picture

Can do a sorta "final last resort protection" ?

Along the lines of this?

if ($test->passes == 0) {
  $test->success = FALSE;
}
jthorson’s picture

Untested, but this should help. Will validate once I find a test with consistent FATALs.

jthorson’s picture

HEAD just passed with a PDO exception, so adding that check too.

jthorson’s picture

Bah ... Typos. :(

jthorson’s picture

Status: Active » Needs review
jthorson’s picture

I believe we've sorted this one with the introduction of #1565718: Script test runner does not clean after fatal error.

sun’s picture

Status: Needs review » Fixed

Yes, definitely :)

sun’s picture

That said, the core patches are not backported yet:

#1541958: Split setUp() into specific sub-methods
#1563620: All unit tests blow up with a fatal error
#1565718: Script test runner does not clean after fatal error
#1611430: Verbose debug output in unit tests relies on the database

So if anyone wants to see this fixed (properly), then make sure to help with those backports. :)

Status: Fixed » Closed (fixed)

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

xjm’s picture

Status: Closed (fixed) » Active

So, we have an additional cause of "0 passes green" here:
http://drupal.org/node/1705748#comment-6302424
http://qa.drupal.org/pifr/test/309608

jthorson’s picture

Status: Active » Fixed

Let's chase this in http://drupal.org/node/1717868

boombatower’s picture

This is a different case, since it exposes that depending on how the drupal system could break pifr doesn't catch that hey I ran tests, but got no results. Obviously, no tests is fine, but you should be able to assume that 1 testcase should have >= 1 assertions.

boombatower’s picture

Status: Fixed » Active
jthorson’s picture

If it's a different case, can we chase it in a different issue? This issue has been solved, reopened, and solved again ... And a new issue opened at the link above. The only thing the three have in common is a shared symptom.

There's many things which have caused this particular symptom, some in PIFR and some in the test runner. Following this thread is going to be difficult if we try and chase each root cause in the same thread, especially if we have two open issues we want to chase ... The one i linked, and whatever you are proposing (which i didn't quite follow, right now, though i'm heavilly jetlagged and low on sleep.)

In any case, i'd prefer to close this and chase each root cause of this symptom in it's own issue ... And if there's a feature enhancement to be proposed, let's not bury it here.

boombatower’s picture

Just to be clear the issue linked to in #31 does not "fix" this issue...it merely masks it and is entirely unrelated as the fix goes into core etc. What core did was break the way we set variables in core, that is one fix we need in pifr. The result of that change was we ran tests and got 0 results out of database so we marked as pass. Sanity checking that the result makes sense is quite different from fixing the reason it was broken. If we want to open a new issue to fix this issue properly sure, but in the past we fixed several special cases, I am proposing we add a bit of logic that will provide a catch all outside of all the other logic.

|- test runner start
|- check if failed to start
|
|
|-fetch results

what I think we need is

|-did I pass any tests to the runner that I expect to be run? If yes then make sure we expect at least one assertion at the end
|- test runner start
|-check if failed to start
|
|
|-fetch results
|-sanity check based on what we passed in

Just a more encapsulating layer instead of so specific. So fundamentally, this is fixing the same problem.

boombatower’s picture

Assigned: Unassigned » boombatower
Status: Active » Needs review
FileSize
3.05 KB

This code is getting a bitch sketch as I see things have kind been thrown in here. I'm going to file an issue against the new system to double check we have all these edge-cases handled and probably clean it up a bit.

boombatower’s picture

boombatower’s picture

One thing I looked at and want to make sure works in views 6.x-3.x which has a .test file with only abstract tests.
http://drupalcode.org/project/views.git/blob/refs/heads/6.x-3.x:/tests/v...
http://qa.drupal.org/pifr/test/27130

Which is extra fun since bot tries to run tests, but no results are to be expected. The patch should handle this properly.

boombatower’s picture

Forgot that during get_result() one must return a result instead of an error. Again this isn't perfect code, trying to not touch it too much and fix in other issue.

boombatower’s picture

Final patch that I tested for catching the issue with the patch that results in no assertion found in db, a test the has a .test file, but not tests found, and a when nothing is found to even look at. Seems to catch all cases now.

http://qa.drupal.org/pifr/test/312488 "Tests were executed, but no results were found."

Only thing I question is if we like the way it handles http://qa.drupal.org/pifr/test/27130. .test file passed, but no tests to run. It currently errors. The thinking being that the test detection system should almost always be broken if that happens. Having nothing to run in a .test file seems questionable if we want to support that.

boombatower’s picture

Tweaked message text.

sun’s picture

I'm not sure... but this again looks like something that shouldn't be in PIFR at all.

PIFR's simpletest worker code is full of these kind of workarounds. Almost all of them should have been applied to run-tests.sh instead.

E.g., see:

#1683884: Update, simplify, and speedup run-tests.sh for PSR-0, and allow to run all tests of a module
#1700682: run-tests.sh attempts to run abstract/base test classes
#1719530: Add --keep-results flag to run-tests.sh

There's no issue for making run-tests.sh bail out if it was supposed to run at least one test and there are no results, but that would just be a natural progression to above mentioned issues.

boombatower’s picture

I would disagree with that. run-tests.sh performs properly in this case...the results are in the database as the tests run, then when it completes they are removed since it printed summaries as it went along. That's exactly what it was told to do. This is merely a sanity check to make sure that the setting to ask it to keep the results (soon to be flag) properly worked and that nothing else catastrophic happened. The other cases in pifr check for specific error codes/messages from run-tests.sh to print them in the log and handle them properly.

There is nothing you can add to run-tests.sh to handle this situation since run-tests.sh is not at fault and performed correctly.

sun’s picture

Why do you think that PIFR should catch errors that run-tests.sh does not catch?

I fail to see what makes PIFR a special user of run-tests.sh.

As a developer, I'd like to get the identical errors on my local box, so I can reproduce the "WTF?" that PIFR might be reporting.

boombatower’s picture

I would agree with getting identical errors, but how can we do that when this is outside the scope of a normal developer. A normal developer will a) not use the flag/variable, and b) if they do will set it using the proper API and if not will get no results in db (correctly). Lets walk through what's happening

- PIFR tries to set clear results variable to FALSE using the d7 method (which no longers works)
- run-tests.sh is executed and does not see the new value since it was not set correctly
- run-tests.sh clears the results based on the value of the variable and does so correctly

PIFR is the only one that knows that the end result is not what it expected. The flag will make this less likely to ever occur again, but and PIFR will also have a catch-all check.

Short of run-tests.sh being able to read minds I do not see how this is possible or even makes sense.

boombatower’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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