Replace multiple test methods in InfoParserUnitTest with 1 testInfoException and a dataprovider

I have rerolled. I then added a string key for each test case in the new data provider plus some other cleanup for readability. I have not included the diff because it is 3x larger than the patch and the previous patch had out of scope changes.

Now that I have completed this I see the Novice tag. Not sure it was suitable, the out of scope changes were confusing things. Anyway, it is done now.

Tried to fix CCF for #29.

Reroll seems good.

Thanks! Fixing a super trivial nit. Leaving RTBC.

The last submitted patch, 32: 3119761-32.patch, failed testing. View results →

Unrelated failure.

1) Drupal\Tests\system\Functional\System\CronRunTest::testAutomatedCron
Failed asserting that 1691374878 is less than 1691374878.

Retesting

Tests passing, restoring RTBC.

@dww, thanks for fixing the capitalization.

I'm triaging RTBC issues → .

Ah, I made the patch here. The IS is still correct and there have been no questions unanswered. This is a bit of refactoring and I think can remain at RTBC.

Got 20% into a review before falling asleep; here's my notes before I forget...

The diff in Dreditor is wonky; this is much easier to review locally. Locally, it's clear that the following methods are being deleted:

• testInfoParserMissingKey()
• testMissingCoreVersionRequirement()
• testMissingCoreVersionRequirement()
• testInfoParserMissingKeys()

Add some details to the IS

The last submitted patch, 32: 3119761-32.patch, failed testing. View results →

Restoring status.

Finally finished the first pass of my review.

1. I don't understand why we still have testInfoParserBroken() in addition to testInfoException(). Maybe someone can explain?
    

2. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -52,94 +52,116 @@ public function testInfoParserNonExisting() {
   +  public function testInfoException($yaml, $expected_exception_message) {
   ...
   +  public function providerInfoException() {
   

   We can add parameter and return typehints to the method signatures.

3. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -52,94 +52,116 @@ public function testInfoParserNonExisting() {
   +    // Set the expected exception for the 2nd call to parse().
   

   Nit: We should spell out "second".

4. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -52,94 +52,116 @@ public function testInfoParserNonExisting() {
   +      $this->assertSame($exception_message . '.info.txt', $exception->getMessage());
   +
   +      $this->infoParser->parse(vfsStream::url("modules/fixtures/broken-duplicate.info.txt"));
   

   Where is there a newline in the middle of the catch block but nowhere around the try/catch? It's not technically wrong, but it's weird and made me initially think the parse() call was outside the try/catch.

   Edit: Looks like this is in HEAD as well and these are just moved lines. Same for the point above about "2nd". I leave it up to contributors whether to fix this, since the diff is already an illegible mess and it's best reviewed with before and after side-to-side (which I will be doing later).

5. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -52,94 +52,116 @@ public function testInfoParserNonExisting() {
   +name: File
   +version: VERSION
   +type: module
   +dependencies:
   +  - self_awareness
   ...
   -name: Skynet
   

   Ahem. The module is supposed to be called Skynet. Thence the dependency on self_awareness. I don't care if it's not a dictionary word! :P

6. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -52,94 +52,116 @@ public function testInfoParserNonExisting() {
   +  - llama_detector
   +  - alpaca_detector
   

   Hi Wim (or someone giving homage thereto).

7. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -52,94 +52,116 @@ public function testInfoParserNonExisting() {
   -   * Tests that a missing 'core_version_requirement' key is detected.
   +   * Test if correct exception is thrown for a broken info file.
   

   This is not quite a complete sentence. I'd suggest:

   Tests that the correct exception is thrown for a broken info file.

Thanks!

Saving credits.

#41
1. Yea, that is a bit unusual. I think it all comes down to the face that two of the original methods,testMissingCoreVersionRequirement and testInfoParserMissingKey, make duplicate files and then test parsing twice. With testInfoParserBroken the YAML itself is broken, so the parser can't examine the data, and the exception message is of a different pattern. The new test method continues to use the testing of two info files.
I found that the duplicate stuff was added in #2313917-203: Core version key in module's .info.yml doesn't respect core semantic versioning → but I haven't read that properly because it is too late here.
2. Yes, I forgot that. Done now.
3. Done.
4. For future legibility, I have removed the blank line in the catch block.
5. I have restored Skynet, but it will come up again in a spelling issue.
6. Removed this test, it was actually a duplicate. As you mentioned the diff on this is messy.
7. Done.

Appears all points have been answered.

@xjm does #43.1 answer your question?

The last submitted patch, 43: 3119761-43.patch, failed testing. View results →

Unrelated test failure, retesting.

1) Drupal\Tests\toolbar\FunctionalJavascript\ToolbarActiveTrailTest::testToolbarActiveTrail with data set #1 ('horizontal')
Failed asserting that false is true.

Seems rerun was successful

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Tests were failing in the MR pipeline. I pushed a commit that should fix them.

Replacement seems good. Meaning no coverage appears to have been lost.

- Rebased to 11.x.
- Fixed 2 of the comments from @longwave in testInfoParserBroken().
- The 4 threads about testInfoException() still need help.

Finished addressing all threads. There's 1 I'm not sure about (see MR comments). Otherwise, this is ready for re-review.

Thanks,
-Derek

p.s. x-post with mondrake, but I noticed the same thing. That thread can also be resolved.

Seems all feedback has been addressed. There's 1 open thread though @longwave mind taking a look?

All feedback was resolved.

Committed and pushed 9f6b172c61 to 11.x and 2a98f61317 to 10.3.x and a9b2d321a4 to 10.2.x. Thanks!

Backported to 10.2.x to keep tests aligned.

quietone → closed merge request !6061

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