Problem description:

On WAMP machines, the TestDiscovery does not correctly parse the line endings. The result is a bunch of /n's in a long string of the test class description. I attached a screenshot, it's huge so I won't embed it here.

Proposed solution:

Fix the normalization of line endings to work on windows.
Now:

 // Normalize line endings.
    $doc_comment = preg_replace('/\r\n|\r/', '\n', $doc_comment);

My patch:

    // Normalize line endings.
    $doc_comment = str_replace('\r\n', '\n', $doc_comment);
    $doc_comment = str_replace('\r', '\n', $doc_comment);

And a screenshot of the fixed version is attached and linked.

UI Changes:

On windows, the Test discovery works again! See the screenshot above.

The rest:

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LKS90 created an issue. See original summary.

LKS90’s picture

Issue summary: View changes
LKS90’s picture

Status: Active » Needs review
FileSize
724 bytes

Here is the patch I talked about in the issue.

LKS90’s picture

Just saw the difference in those two lines, here is a corrected patch.

LKS90’s picture

And with the last patch I made another stupid change. Fixed here...

LKS90’s picture

LKS90’s picture

Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

Version: 8.9.x-dev » 9.1.x-dev
Component: simpletest.module » base system
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative, +Needs reroll

Needs a reroll for 9.1.x as code moved to core/lib/Drupal/Core/Test/TestDiscovery.php.

ridhimaabrol24’s picture

Status: Needs work » Needs review
FileSize
722 bytes

Rerolled patch for 9.1.x! Please review!

Hardik_Patel_12’s picture

Issue tags: -Needs reroll
Kristen Pol’s picture

Status: Needs review » Needs work
Issue tags: +Windows, +Global2020

Thanks for the update.

1) Tests pass.

2) Looked for other usage of preg_replacewith a similar structure but didn't find any.

3) Looked at usage of str_replace in core, it might make sense to combine these for brevity so instead of:

+++ b/core/lib/Drupal/Core/Test/TestDiscovery.php
@@ -395,7 +395,8 @@ public static function getTestInfo($classname, $doc_comment = NULL) {
-    $doc_comment = preg_replace('/\r\n|\r/', '\n', $doc_comment);
+    $doc_comment = str_replace('\r\n', '\n', $doc_comment);
+    $doc_comment = str_replace('\r', '\n', $doc_comment);

it's one line like:

$doc_comment = str_replace(['\r\n', '\r'], ['\n', '\n'], $doc_comment);

For example, that was the approach used in core/lib/Drupal/Component/Utility/Variable.php:

$var = str_replace(['\\', '$', '"', "\n", "\r", "\t"], ['\\\\', '\$', '\"', '\n', '\r', '\t'], $var);
ridhimaabrol24’s picture

Status: Needs work » Needs review
FileSize
679 bytes
621 bytes

Hi @Kristen pol made the changes as suggested in #19. Please review!

Manav’s picture

Assigned: Unassigned » Manav
Status: Needs review » Reviewed & tested by the community

Patch #20 works fine for me.

RTBC

Manav’s picture

Assigned: Manav » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Here's why this is happening - https://3v4l.org/hTIOV

So one fix would be to do $doc_comment = preg_replace('/\r\n|\r/', "\n", $doc_comment); - note the double quotes.

If we want to go with the str_replace method then lets do $doc_comment = str_replace(["\r\n", "\r"], "\n", $doc_comment);

Note that we can add a test to \Drupal\Tests\Core\Test\TestDiscoveryTest::infoParserProvider with alternate line-ending to prove we've fixed this.

shetpooja04’s picture

Assigned: Unassigned » shetpooja04
shetpooja04’s picture

I have made the changes, please review

shetpooja04’s picture

Assigned: shetpooja04 » Unassigned
Status: Needs work » Needs review
Kristen Pol’s picture

Status: Needs review » Needs work

Thanks for the update. I've confirmed that the change addresses the comment from #23.

Back to needs work for adding test. See #23 for where the test can be added.

raman.b’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
1.09 KB

Adding test case for doc comment with a mix of CRLF, CR, and LF line terminators.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

@raman.b, thanks for the test.

Nearly done. Just a few things. And can we have a fail patch as well.

  1. +++ b/core/tests/Drupal/Tests/Core/Test/TestDiscoveryTest.php
    @@ -264,6 +264,28 @@ public function infoParserProvider() {
    +    // Multi-line summary with mix of carriage return and line feed line endings.
    

    Line > 80 chars

  2. +++ b/core/tests/Drupal/Tests/Core/Test/TestDiscoveryTest.php
    @@ -264,6 +264,28 @@ public function infoParserProvider() {
    +      "/**
    + * Tests the Simpletest UI internal browser. And the summary line with mix of
    + * carriage return and line feed line endings continues and
    + * there is no gap to the annotation.
    + *
    + * @group simpletest
    + */
    

    The characters \n and \r should be visible to the reader. Perhaps change to a double quoted string. Since this is a WAMP issue the string should contain at least one "\r\n" and one "\r" to test the change. It does not have a "\r\n".

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

@smustgrave asked for clarification of #31.2, so I am attempting that now. When reading the new doc block added in the test in my IDE I cannot see the non printable characters "\n" and "\r" characters to confirm the test string is formed correctly for the test. Something like " * carriage return and line feed line endings continues and\r\n * there is no gap to the annotation." would make it easier to review. And it turns out there is no, "\r\n" in the new string. Therefor, the test is not testing what the comment says it is testing.

Plus, the new string does not cause a failure with the existing code.

smustgrave’s picture

The last submitted patch, 35: 2548961-35-tests-only.patch, failed testing. View results

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
144 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

ranjith_kumar_k_u’s picture

Rerolled #35

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Hardik_Patel_12’s picture

The test is unsuccessful because the code $info = TestDiscovery::getTestInfo($classname, $doc_comment); is returning "PHPUnit-" as the type in core/tests/Drupal/Tests/Core/Test/TestDiscoveryTest.php ,while the expectation is for simpletest.