Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Currently, the comment for the BROWSERTEST_OUTPUT_BASE_URL
reads:
<!-- To have browsertest output use an alternative base URL. For example if
SIMPLETEST_BASE_URL is an internal DDEV URL, you can set this to the
external DDev URL so you can follow the links directly.
-->
This can be improved in a few ways:
- Use the official name for DDEV-Local (or don't use it at all)
- Briefly explain what it is
- Explain a little more about the situation where you'd use this
I'm going to propose a variant that doesn't mention ddev, since I think a core phpunit.xml environment variable should be able to explain itself independently:
<!-- By default, browser tests will output links that use the base URL set in SIMPLETEST_BASE_URL. However, if your SIMPLETEST_BASE_URL is an internal path (such as may be the case in a virtual or Docker-based environment), you can set the base URL used in the browser test output links to something reachable from your host machine here. This will allow you to follow them directly and view the output. -->
Let's agree on the wording first, then we can write a patch.
Comment | File | Size | Author |
---|---|---|---|
#20 | 3159842-9.5-20.patch | 1.23 KB | andypost |
#19 | 3159842-19.patch | 1.15 KB | andypost |
#19 | interdiff.txt | 1.25 KB | andypost |
Comments
Comment #2
wizonesolutionsComment #6
smustgrave CreditAttribution: smustgrave at Mobomo commentedBrought this up in Drupal slack and seems the wording makes sense. Please submit a patch.
Comment #7
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #8
longwaveCan we wrap the lines in the comment at 80 chars, like the other comments in that file?
Comment #9
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #10
andypostit's longer then 80
Comment #11
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo that file has other instances of over 80 characters unreleated to this ticket. Should we fix those too?
Comment #12
longwaveNot in this issue, let's do that separately.
Comment #13
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #14
ressa CreditAttribution: ressa at Ardea commentedShouldn't there be a test to keep comment line length at max. 80 characters, if it is a requirement?
Comment #15
longwaveThis is a bit of an edge case given that it's an XML configuration file and we only have two of those, but we do have coding standards rules for line lengths - so probably not worth adding a test for, but we should still apply our standards.
Comment #16
ressa CreditAttribution: ressa at Ardea commentedThanks for clarifying. I just assumed that if there were a test for comment length, that it would run on any file, and catch it automatically. But I agree, if there's only two files of this type, it would be overkill to create tests for comment length, for just those two.
Comment #17
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo this is good for review still?
Comment #18
longwaveThis is still longer than 80 chars. I also think we can drop the last sentence, as it's kinda repeating what came before.
Comment #19
andypostfixed it
Comment #20
andypostHEAD is broken, patch for 9.5
Comment #21
longwaveThanks, looks good to me now.
I could still take or leave the last sentence "This will allow you to follow them directly and view the output." as it doesn't add anything for me, but not worth nitpicking over that.
Comment #22
alexpottCommitted and pushed 3112fc9e97 to 10.1.x and 3b5de12d11 to 10.0.x. Thanks!
Committed and pushed 30cb762a43 to 9.5.x and 884aaabeb1 to 9.4.x. Thanks!