Allow run-tests.sh to report skipped/incomplete PHPUnit tests

Cant we add another status of "skipped" or something?

In a perfect world, I would have a way to run just the tests that derive from PHPUnit, and the ones that are simpletest tests separately.

Then, I would process the simpletest xml separately from the phpunit xml.

I suggest we not care about this

If you're suggesting, lets fix this, but only where it makes sense to, then yeah, +1.

skipped/risky is some valueable data to provide from a CI side because it might also help us solve issues like this one: #2065537: Test class without any methods silently ignored by testbot?

@Mile23 I think we should pursue catching skipped tests and risky tests now. Since we using PHPUnit 6 for the majority of Drupal 8 test runs now. It's only that the PHPUnit 4 based test runs won't error for this because patch testing is done on PHPUnit 6.

Just some quick review

1. +++ b/core/modules/simpletest/tests/src/Unit/SimpletestJUnitDeprecationTest.php
   @@ -0,0 +1,54 @@
   +/**
   + * @group simpletest
   + * @group legacy
   + */
   +class SimpletestJUnitDeprecationTest extends UnitTestCase {
   +
   +  protected function setUp() {
   +    parent::setUp();
   +    require_once __DIR__ . '/../../../simpletest.module';
   +  }
   +
   

   Can we ensure to run this test in isolation?

2. +++ b/core/tests/Drupal/Tests/Core/Test/JUnitHelperTest.php
   @@ -0,0 +1,87 @@
   +    $this->assertEquals(count($records), 4, 'All testcases got extracted');
   

   You can use assertCount instead

+++ b/core/modules/simpletest/simpletest.module
@@ -774,8 +782,27 @@ function simpletest_mail_alter(&$message) {
+  // Use the new JUnit helper if we're using a suitable version of PHPUnit.
+  if (version_compare($phpunit_version, '6.0.0', '>')) {
+    $helper = new JUnitHelper($test_id, $phpunit_xml_file);
+    return $helper->getRecords() ?: NULL;
+  }

@@ -838,6 +871,11 @@ function simpletest_phpunit_find_testcases(\SimpleXMLElement $element, \SimpleXM
  *
  * @return array
  *   An array containing the {simpletest} result row.
+ *
+ * @deprecated in Drupal 8.6.x for removal before Drupal 9.0.0. Use
+ *   \Drupal\Core\Test\JUnitHelper to convert JUnit to simpletest database rows.
+ *
+ * @see https://www.drupal.org/node/2966954
  */
 function simpletest_phpunit_testcase_to_row($test_id, \SimpleXMLElement $testcase) {

What about changing the documentation of simpletest_phpunit_testcase_to_row to document that this is now for phpunit 5.5 only? Maybe this makes it a tiny little bit easier to understand what is going on.

@berdir pointed me here in Slack. Thanks for all the progress so far! Sadly, this no longer applies to 8.7.x.

On 8.6.x, I'm using the CLI, not the simpletest UI. Patch works like a charm.

If I forgot to start or intentionally kill my chromedriver, run-tests.sh would silently pass all the FunctionalJavascript tests. Even when I put $this->assertTrue(FALSE, 'this better fail'); in the test function. ;)

Before:

Test summary
------------

Drupal\Tests\system\FunctionalJavascript\OffCanvasTest         6 passes

Test run duration: 47 sec

Detailed test results
---------------------

---- Drupal\Tests\system\FunctionalJavascript\OffCanvasTest ----

Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Pass      Other      OffCanvasTest.php   33 Drupal\Tests\system\FunctionalJavas
Pass      Other      OffCanvasTest.php   33 Drupal\Tests\system\FunctionalJavas
Pass      Other      OffCanvasTest.php   33 Drupal\Tests\system\FunctionalJavas
Pass      Other      OffCanvasTest.php   33 Drupal\Tests\system\FunctionalJavas
Pass      Other      OffCanvasTest.php   33 Drupal\Tests\system\FunctionalJavas
Pass      Other      OffCanvasTest.php  114 Drupal\Tests\system\FunctionalJavas

With #28 applied, I now see this:

Test summary
------------

Drupal\Tests\system\FunctionalJavascript\OffCanvasTest         0 passes             6 skipped/incomplete

Test run duration: 46 sec

Detailed test results
---------------------

I can't comment on the simpletest UI parts, but the CLI looks like a big win. This is a major WTF from my POV. Escalating this to a bug. Silently passing a whole set of tests is really, really evil. At least telling me all 6 test cases were skipped is a huge improvement.

Thanks @Mile23 for fixing this!
-Derek

Thanks and thanks! Following the parent [meta] and The Fate issues now.

Cheers,
-Derek

+++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
@@ -116,4 +117,33 @@ public function testUserAgentValidation() {
+  /**
+   * Tests that PHPUnit and KernelTestBase tests work through the UI.
+   */
+  public function testTestingThroughUI() {
+    $this->drupalGet('admin/config/development/testing');
+    $this->assertTrue(strpos($this->drupalSettings['simpleTest']['images'][0], 'core/misc/menu-collapsed.png') > 0, 'drupalSettings contains a link to core/misc/menu-collapsed.png.');
+    // We can not test WebTestBase tests here since they require a valid .htkey
+    // to be created. However this scenario is covered by the testception of
+    // \Drupal\simpletest\Tests\SimpleTestTest.
+
+    $tests = [
+      // A KernelTestBase test.
+      'Drupal\KernelTests\KernelTestBaseTest',
+      // A PHPUnit unit test.
+      'Drupal\Tests\action\Unit\Menu\ActionLocalTasksTest',
+      // A PHPUnit functional test.
+      ThroughUITest::class,
+    ];
+
+    foreach ($tests as $test) {
+      $this->drupalGet('admin/config/development/testing');
+      $edit = [
+        "tests[$test]" => TRUE,
+      ];
+      $this->drupalPostForm(NULL, $edit, t('Run tests'));
+      $this->assertText('0 fails, 0 skipped, 0 exceptions');
+    }
+  }

I think this is being added back by mistake.

Patch still applies on 8.8.x, where's there's no longer PHP 5 nor PHPUnit 4. Skipped and incomplete tests are reported together, whereas risky ones just result as fails since #3059332: Mark kernel tests that perform no assertions as risky. What's left to RTBC here?

I am working on trying to expose 'risky' and 'incomplete' test results separately.

Hmm, #43 is complicated... PHPUnit's PHPUnit\Util\Log\JUnit reports 'skipped' and 'incomplete' tests in one and the same way via doAddSkipped, and 'risky' ones as 'errors', so it's not straightforward to split them from the JUnit log. Actually @Mile23 already hit that in #8. I could not find ways to instruct PHPUnit to use a different class for JUnit reporting, any ideas?

This is adding an additional test listener. It is c/p of the PHPUnit Junit logger - but without extending from Printer. The listener accumulates the test run statistics, and on destruction flushes the XML file. 'risky', 'skipped' and 'incomplete' cases are all managed with an additional messageattribute in the <skipped> element of the DOM testcase element - that seems to be allowed in the specs https://llg.cubic.org/docs/junit/.

No additional tests yet, nor changes to existing ones, so it will probably fail quite a lot...

Just some progress.

This should account separately for 'incomplete' tests now. I increased the size of the {simpletest}.status column to 12 to allow the text 'incomplete' to fit in.

The failure in #54 is very tricky - Drupal\Tests\simpletest\Functional\SimpletestUiTest::testTestingThroughUI is a functional test testing that the simpletest UI performs correctly some tests. That means that run-test.sh is called in multiple nesting, once while running the test itself, and once within the test to allow the simpletest form processing the sub-test. I'm quite sure this impacts the approach to pass the junit file path as an environment variable, but the hack I attempted have not been able to get it sorted so far. It's very difficult to try and pass dynamic variables to PHPUnit, unfortunately.

Any idea?

Great. I was just thinking to try and use Symfony’s Process component that allows broader control on environment variables passed to spawned processes. And replace the call to exec with that.

Maybe I got the problem - since we no longer pass the junit result file path via the --log-junit option, but rather as an environment variable SIMPLETEST_JUNIT_FILE, the directory where that file should be written by the test started within the UI form is no longer prepared. Working on a patch that will also include moving to Symfony Process component.

This should fix #54, finally. It will break other stuff but that should be easier to fix later. Certainly more test will be required.

This is introducing Symfony Process to execute the spawned PHPUnit runs, in place of prior exec calls.

Hopefully, this should come back green.

Only code style cleanup here, the c/p of the JUnit listener was using a different CS standard.

DRYed a bit the JUnitListener implemenation and cleaned up CS issues.

@Mile23 thanks for review!

#67.1 - I'd daresay that not even PHPUnit's JUnit XML is strictly Ant JUnit compliant: they report 'assertions' as an attribute of the <testsuite> which is not in the specs. Anyway, I agree - we have our own format here now and it's worth documenting it thoroughly.
#67.2 - I disagree with that. Adding PHPUnit extensions (listener, printer classes and traits) in the same namespace of the test is prone to issues, because they get discovered by PHPUnit when it runs --testsuite(s) or --group(s). This causes problems like the ones in #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself and #2950132-74: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5. IMHO we should go in the opposite direction of moving the existing classes to Drupal\Core\Test, or even better have a totally separate namespace for PHPUnit extensions (so that they won't collide either with tests or with SUT). We need framework manager review here, flagging.
#67.3 - exec returns "The last line from the result of the command.", whereas $process->getOutput() will return the entire output. Is it worth extracting the last line to return it given it's not used anywhere?

More todos:
1. We need unit tests for the JUnitListener itself.
2. With this change a testsuite containing a risky test is reported green by DrupalCI and SimpletestUI form. I believe it should be reported as a fail, though.
3. We need unit tests for the JUnitHelper processing XML with incomplete and with risky tests - only 'skipped' are tested so far
4. Is it worth to summarise also PHPUnit assertions count in run-tests.sh and Simpletest UI?
5. I noticed that if a testsuite has all the tests marked skipped, it is missing the class name in run-test.sh report (https://dispatcher.drupalci.org/job/drupal_patches/1346/consoleFull - line below Drupal\Tests\Core\Discovery\YamlDiscoveryTest)

Interestingly, the PHP 7.3.x-dev test run in #65 has a significantly lower number of passes compared to other PHP versions - looking at the log it is due to 2366 skipped tests in Drupal\Tests\Component\Serialization\YamlTest. I suppose it's an indication that the PHP 7.3 image on the bots have not the PHP yaml extension installed.

One thing to consider, and Im not sure where it goes, but the testbots are currently reliant on a very limited junit parser tool because the more advanced one introduced a requirement for an xsl because it insisted that we must have a valid schema for the junit xml because the maintainer of said plugin decided they were tired of troubleshooting other peoples broken xml.

This caused us to have to revert to the more limited parser, which sabotaged a lot of other things we were doing, like the ability to have no tests but still run phpcs.

So, whatever schema we end up with, it would be *really* nice if there were a corresponding .xsl file that I can use on the testbots to validate the xml.

We can base it off of the Ant junit one: https://github.com/windyroad/JUnit-Schema/blob/master/JUnit.xsd but what this plugin wants us to do is use an .xsl stylesheet to convert what we have to standard junit: https://wiki.jenkins.io/download/attachments/38633556/input.xsl?version=...

Just a reroll of #66 after commit of #2863055: Move TestDiscovery out of simpletest module, minimize dependencies.

#72 sure. I'll try to keep up with those changes as they happen to avoid major headache later.

Reroll after #3074043: Move simpletest.module DB-related functions to TestDatabase, deprecate

Rerolled after #2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate, this was a tough one, let's see

A reminder, we need to add #67, #68, #69, #70 in the IS todos (and do them :)). But waiting for the #3057420: [meta] How to deprecate Simpletest with minimal disruption merry-go-round to settle before tacking that. In the meantime, ... rerolls.

Addressing failures of #76.

Maybe not worry about #70 so much. I think I have a solution for jenkins now.

Just a reroll to keep up with HEAD.

Looking at the changes to TestDatabase here:

+++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -341,10 +341,10 @@ public static function testingSchema() {
         'status' => [
           'type' => 'varchar',
-          'length' => 9,
+          'length' => 12,
           'not null' => TRUE,
           'default' => '',
-          'description' => 'Message status. Core understands pass, fail, exception.',
+          'description' => 'Message status.',
         ],

I don't see why we're postponed on #3075608: Introduce TestRun objects and refactor TestDatabase to be testable

Also the comment that was about framework manager input was about where to put stuff that's not a test and should not be part of the system-under-test. We have that now - it's Drupal\TestTools.

This would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

Rerolled, on top of #3075608: Introduce TestRun objects and refactor TestDatabase to be testable.

Listeners' signatures changed since #84.

I cannot figure out how to silence the PHPUnit deprecation message

The "Drupal\Core\Test\JUnitListener" class implements "PHPUnit\Framework\TestListener" that is deprecated Use the `TestHook` interfaces instead.

inserting it in the allowed deprecations list in DeprecationListenerTrait does not seem to have any effect.

#91 we probably need to encapsulate Drupal\Core\Test\JUnitListener in DrupalListener instead of adding it to the listeners in the phpunit.xml file.

Fixing deprecation.

I think it would be useful to expose also the duration of the each test run in the run-test.sh output, something like...

09:43:58 32.4s Drupal\Tests\jsonapi\Functional\ConfigTestTest 7 passes, 1 skipped

thoughts?

Trying #102.

Rebased after #3075608: Introduce TestRun objects and refactor TestDatabase to be testable and changed to MR workflow.

@longwave and I discussed on Slack on whether working on this issue would still make sense given the upcoming move to GitLab testing. We leaned to a no, but would like to get more feedback.

For sure it improves the console log output by showing execution time, passes, skips, incomplete etc., see latest MR test log https://dispatcher.drupalci.org/job/drupal_patches/171125/consoleFull

I'd lean towards a no too.

Would also say no but depends when exactly that switch will happen?

Four leaning no account for at least a full no…

We're relying on run-tests.sh in gitlab CI, and didn't get very far trying to switch to paratest, so this is still relevant.

@catch but now we have the reports in gitlab CI we are finally seeing which tests are skipped. You can see this in the gitlab reports. Unfortunately it does not tell you why but it is more information.

Opened a new branch, just tried to merge the old MR with 11.x. Will fail badly...

@alexpott right but we still print the run-tests.sh output in the job logs, and it'd be better if it matched the test reports in gitlab. I always end up going to the jobs about 95% of the time to see what failed.

In PHPUnit 10+ the Junit log does not separate between skipped and incomplete tests, both are reported as ‘skipped’. Also, there is no specific logic around risky tests.

We can simplify here accordingly.

This is an issue where the review makes more sense when tests fail :)

What's being output currently by run-tests.sh in the 'Detailed test results' section is very simpletest-ish, and possibly needs reconsideration.

1) the 'Group' column in PHPUnit always bears 'Other', so it's useless. I replaced it with the execution time of the single test case.
2) the 'Function' column in PHPUnit is the name of the test, including the name of the dataset provided by a data provider. I renamed the column to 'test'.
3) all reported lines are adding an empty line - it's meant to bear a message, but in PHPUnit passed and skipped test cases never have one. So just chanhed here to skip printing anything if the message is missing.

Also, in HEAD right now if a PHPUnit test run ends with a 'error' exit code, we have a duplicate line reported for the same test class. Fixed here.
Also fixed that if the PHPUnit runner fails for failure/errors, we miss details of testcases passed/failed/errored.

Reviews, based on the output of the MR, appreciated.

Rebased after commit of #3465132: Show test run time by class in run-tests.sh output.

OK, fully green now.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

False positive

rebased

rebased

adding before/after screenshots

@mile23 the tests were not ignored, actually. They were reported as if they passed, which is the original reason of this issue - and a cause of confusion.

PHPUnit tests can be marked as ‘skipped’ or ‘incomplete’ in the test code, but both end up as ‘skipped’ in the JUnit log - that’s a JUnit limitation AFAICS.

I do not think there’s a need for generic followups to unskip tests - that’s very much dependent on the use case and should be decided case by case. For example, database driver specific kernel tests are prepared for all drivers all times, but then those of the drivers that are not of the currently running database get skipped. By design.

in one of those 'after' images, we see a block test marked red. Do we need to file an issue to fix that test?

Aha 😀

Those screenshots actually captured a random test failure… those won’t go away with this MR... 😀

rebased

merged with HEAD

Do we have some steps to demo locally?

Well the steps would be
a) apply the patch
b) break a test so that the failure can be reported, take note of the @group of the test (ideally, in a test group where some tests are marked skipped/incomplete so you can see the skips on top of the failure)
c) run run-tests.sh for the group noted above with --verbose option and enjoy the improvements

I'd give a provisional RTBC but I think the original patch was mine

I think there are so few people who would be able to RTBC this, that someone has to be bold...

Leaning on the screenshots of #134 and reading the summary seems like a good addition. Not entirely sure how to best test it out but since it's been close to 2 months I'm going to mark to see if we can maybe try out on 11.x. Wonder if it's one of those things that will help when actually used.

To meaningfully manual test this, apply the MR patch locally, break some tests, and run tests via run-tests.sh, and llok at results.

Re: #143/#144: Or don't start chromedriver locally and try to run any FunctionalJavascript test.

I don't love having the update in system module, but first of all I thought 'why isn't this in simpletest module' and then I suddenly aged ten years. Unless we don't have the update at all, which would be inconvenient for sites that have this table for whatever reason, even if it's people's local installs, I don't see a way around it.

Apart from that this looks good, and it will unblock more issues in the cluster of issues around modernising run-tests.sh, so let's go ahead here. Functionality looks great from the screenshots and we'll soon see what it's like in gitlab runs too.

Committed/pushed to 11.x, thanks!