Problem/Motivation

Not sure how this was missed or how we didn't discover this earlier. But it looks like we don't have Build added as a test type to bootstrap. See https://www.drupal.org/pift-ci-job/1428057

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3086373-12.patch1.4 KBheddn
#10 3086373-10.patch2.8 KBheddn
#10 interdiff_2-10.txt2.03 KBheddn
#2 3086373.patch657 bytesheddn

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new 3086373.patch657 bytes
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Active » Needs review
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

This is a sensible change - made me go looking for more things where all the test suites are enumerated but we didnt add to them:

core.api.php could use an update:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/core.api.php#L... for example.

Same with core/tests/README.md:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/tests/README.md

A test of phpunit isnt checking for build tests:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/tests/Drupal/T...

Not sure if thats a different followup, or could all be part of this issue.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

+1

heddn’s picture

heddn
Primary language English
Location Illinois
commented

re #4: I've added #3086532: Further document Build and in general improve testing docs for the docs and commented in #2984031: Create Build Tests For Composer and Drupal for the final phpunit test.

With those opened, is this good to go?

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Priority: Normal » Major

Also, bumping priority as this blocks adoption of the new build test type. Without it, we get things like:

PHP Fatal error: Class 'Drupal\Tests\automatic_updates\Build\UpdateTest' not found

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Let's add the dataprovider for build suites here, from #4: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/tests/Drupal/T...

The documentation can be in the followup.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

We don't have any build tests yet that aren't in the component. So until one lands, there isn't any point, no?

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_2-10.txt2.03 KB
new 3086373-10.patch2.8 KB

This should address #8. Although once we add module level tests, we should re-visit. Because until then, we don't really have anything to test.

I also improved/fixed the existing tests by adding a class_exists call. Which then meant some other unrelated test types needed fixing. If we'd rather, I can move those to a follow-up.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Test/TestDiscoveryTest.php
@@ -512,13 +512,14 @@ public function testGetTestsInProfiles() {
+    $this->assertTrue(class_exists($classname));
...
-    $data['simpletest-webtest'] = ['\Drupal\rest\Tests\NodeTest', FALSE];
-    $data['simpletest-kerneltest'] = ['\Drupal\hal\Tests\FileNormalizeTest', FALSE];

@@ -527,8 +528,9 @@ public function providerTestGetPhpunitTestSuite() {
-    $data['core-functionaltest'] = ['\Drupal\FunctionalTests\ExampleTest', 'Functional'];
-    $data['core-functionaljavascripttest'] = ['\Drupal\FunctionalJavascriptTests\ExampleTest', 'FunctionalJavascript'];

Yah, sorry, I should have been clearer. We don't want to assert whether the class exists, because we do want it to return FALSE on some of them (since a simpletest test shouldn't be in a suite).

So revert the rest of the test and then add a build class data set so we know we can get a 'Build' back from TestDiscovery::getPhpunitTestSuite().

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
StatusFileSize
new 3086373-12.patch1.4 KB

No interdiff. I'm still confused though how these two things are related. They don't seem to be.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Missing bootstrap of Build tests » Build test framework cleanup: bootstrap and tests
Status: Needs review » Reviewed & tested by the community

Thanks, @heddn.

Ok, let's rescope a little.

The patch looks good, but the tests haven't completed. RTBC, the testbot will unset it if there's a fail, though.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Whoopsie. :P

Committed and pushed to 9.0.x; cherry-picked to 8.9.x and 8.8.x. Thanks!

  • webchick committed 7471e59 on 9.0.x 
    Issue #3086373 by heddn, Mile23, Mixologic: Build test framework cleanup...

  • webchick committed 9094ab8 on 8.9.x 
    Issue #3086373 by heddn, Mile23, Mixologic: Build test framework cleanup...

  • webchick committed 29aa738 on 8.8.x 
    Issue #3086373 by heddn, Mile23, Mixologic: Build test framework cleanup...

Status: Fixed » Closed (fixed)

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