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:

  1. Use the official name for DDEV-Local (or don't use it at all)
  2. Briefly explain what it is
  3. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wizonesolutions created an issue. See original summary.

wizonesolutions’s picture

Title: Fix wording in BROWSERTEST_BASE_URL comment » Fix wording in BROWSERTEST_OUTPUT_BASE_URL comment

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

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.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.1.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should 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.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

smustgrave’s picture

Status: Needs review » Needs work

Brought this up in Drupal slack and seems the wording makes sense. Please submit a patch.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
longwave’s picture

Status: Needs review » Needs work

Can we wrap the lines in the comment at 80 chars, like the other comments in that file?

smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
andypost’s picture

it's longer then 80

smustgrave’s picture

So that file has other instances of over 80 characters unreleated to this ticket. Should we fix those too?

longwave’s picture

Not in this issue, let's do that separately.

smustgrave’s picture

ressa’s picture

Shouldn't there be a test to keep comment line length at max. 80 characters, if it is a requirement?

longwave’s picture

This 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.

ressa’s picture

Thanks 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.

smustgrave’s picture

So this is good for review still?

longwave’s picture

Status: Needs review » Needs work

This is still longer than 80 chars. I also think we can drop the last sentence, as it's kinda repeating what came before.

andypost’s picture

andypost’s picture

HEAD is broken, patch for 9.5

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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!

  • alexpott committed 3112fc9 on 10.1.x
    Issue #3159842 by smustgrave, andypost, longwave: Fix wording in...

  • alexpott committed 3b5de12 on 10.0.x
    Issue #3159842 by smustgrave, andypost, longwave: Fix wording in...

  • alexpott committed 30cb762 on 9.5.x
    Issue #3159842 by smustgrave, andypost, longwave: Fix wording in...

  • alexpott committed 884aaab on 9.4.x
    Issue #3159842 by smustgrave, andypost, longwave: Fix wording in...

Status: Fixed » Closed (fixed)

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