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.
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.
On windows, the Test discovery works again! See the screenshot above.
None
| Comment | File | Size | Author |
|---|---|---|---|
| #39 | 2548961-39.patch | 1.86 KB | ranjith_kumar_k_u |
| #38 | 2548961-nr-bot.txt | 144 bytes | needs-review-queue-bot |
| #35 | 2548961-35.patch | 1.89 KB | smustgrave |
| #35 | 2548961-35-tests-only.patch | 1.24 KB | smustgrave |
| #35 | interdiff-28-35.txt | 878 bytes | smustgrave |
Comments
Comment #2
LKS90 commentedComment #3
LKS90 commentedHere is the patch I talked about in the issue.
Comment #4
LKS90 commentedJust saw the difference in those two lines, here is a corrected patch.
Comment #5
LKS90 commentedAnd with the last patch I made another stupid change. Fixed here...
Comment #6
LKS90 commentedComment #7
LKS90 commentedComment #16
kristen polNeeds a reroll for 9.1.x as code moved to
core/lib/Drupal/Core/Test/TestDiscovery.php.Comment #17
ridhimaabrol24 commentedRerolled patch for 9.1.x! Please review!
Comment #18
hardik_patel_12 commentedComment #19
kristen polThanks 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_replacein core, it might make sense to combine these for brevity so instead of:it's one line like:
For example, that was the approach used in
core/lib/Drupal/Component/Utility/Variable.php:Comment #20
ridhimaabrol24 commentedHi @Kristen pol made the changes as suggested in #19. Please review!
Comment #21
manav commentedPatch #20 works fine for me.
RTBC
Comment #22
manav commentedComment #23
alexpottHere'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.
Comment #24
shetpooja04 commentedComment #25
shetpooja04 commentedI have made the changes, please review
Comment #26
shetpooja04 commentedComment #27
kristen polThanks 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.
Comment #28
raman.b commentedAdding test case for doc comment with a mix of
CRLF,CR, andLFline terminators.Comment #31
quietone commented@raman.b, thanks for the test.
Nearly done. Just a few things. And can we have a fail patch as well.
Line > 80 chars
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".
Comment #34
quietone commented@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.
Comment #35
smustgrave commentedGave it a shot.
Comment #38
needs-review-queue-bot commentedThe 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.
Comment #39
ranjith_kumar_k_u commentedRerolled #35
Comment #41
hardik_patel_12 commentedThe 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.