Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
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 CreditAttribution: LKS90 at MD Systems GmbH commentedComment #3
LKS90 CreditAttribution: LKS90 at MD Systems GmbH commentedHere is the patch I talked about in the issue.
Comment #4
LKS90 CreditAttribution: LKS90 at MD Systems GmbH commentedJust saw the difference in those two lines, here is a corrected patch.
Comment #5
LKS90 CreditAttribution: LKS90 at MD Systems GmbH commentedAnd with the last patch I made another stupid change. Fixed here...
Comment #6
LKS90 CreditAttribution: LKS90 at MD Systems GmbH commentedComment #7
LKS90 CreditAttribution: LKS90 at MD Systems GmbH commentedComment #16
Kristen PolNeeds a reroll for 9.1.x as code moved to
core/lib/Drupal/Core/Test/TestDiscovery.php
.Comment #17
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x! Please review!
Comment #18
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedComment #19
Kristen PolThanks for the update.
1) Tests pass.
2) Looked for other usage of
preg_replace
with 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:it's one line like:
For example, that was the approach used in
core/lib/Drupal/Component/Utility/Variable.php
:Comment #20
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedHi @Kristen pol made the changes as suggested in #19. Please review!
Comment #21
Manav CreditAttribution: Manav as a volunteer commentedPatch #20 works fine for me.
RTBC
Comment #22
Manav CreditAttribution: Manav as a volunteer 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 CreditAttribution: shetpooja04 at QED42 commentedComment #25
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedI have made the changes, please review
Comment #26
shetpooja04 CreditAttribution: shetpooja04 at QED42 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 CreditAttribution: raman.b at OpenSense Labs commentedAdding test case for doc comment with a mix of
CRLF
,CR
, andLF
line terminators.Comment #31
quietone CreditAttribution: quietone as a volunteer 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 CreditAttribution: quietone at PreviousNext 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 CreditAttribution: smustgrave at Mobomo commentedGave it a shot.
Comment #38
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer 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 CreditAttribution: ranjith_kumar_k_u as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled #35
Comment #41
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 for Drupal India Association 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.