Make WordPress Core

Opened 5 weeks ago

Last modified 3 weeks ago

#49050 reopened defect (bug)

skipOnAutomatedBranches() does not work as expected

Reported by: SergeyBiryukov Owned by: SergeyBiryukov
Milestone: 5.3.3 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: fixed-major
Focuses: Cc:
PR Number:


Background: #49049

WP_UnitTestCase_Base::skipOnAutomatedBranches() is supposed to make sure some tests only run on trunk/master, not on other branches.

test_readme() is one of those tests, however it currently runs (and fails) at least on the 5.3 branch too, see job 268747198 for example.

Change History (6)

#1 @SergeyBiryukov
5 weeks ago

Originally introduced in [40241] / #39486.

Just noted that this only applies to the 5.3 branch. For 5.2 and earlier branches, the tests are skipped as expected, as job 266659075 shows.

This appears to be caused by the work on #47767. It looks like TRAVIS_BRANCH and TRAVIS_PULL_REQUEST environment variables are not passed to the Docker container.

Looking closer, the skip condition in ::skipOnAutomatedBranches() could use some further correction. Per Travis documentation, if the current job is not a pull request, TRAVIS_PULL_REQUEST contains a string "false", not boolean false, so false !== $travis_pull_request always returns true, whether the job is a pull request or not.

If the goal is to skip on pull requests OR branches, this seems to be the correct condition:

'false' !== $travis_pull_request || 'master' !== $travis_branch

#2 @SergeyBiryukov
5 weeks ago

In 46999:

Build/Test Tools: Pass the TRAVIS_BRANCH and TRAVIS_PULL_REQUEST environment variables along to the Docker container.

This ensures that WP_UnitTestCase::skipOnAutomatedBranches() has access to these variables.

See #49050, #47767.

#3 @SergeyBiryukov
5 weeks ago

The function_exists( 'getenv' ) check in line 190 also seems redundant, as we already have two other instances of getenv() without that check in lines 82 and 99 of includes/bootstrap.php.

WordPress core also uses it without checking in wp_get_update_php_url() and wp_get_direct_php_update_url().

Last edited 5 weeks ago by SergeyBiryukov (previous) (diff)

#4 @SergeyBiryukov
5 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 47000:

Tests: Correct the check for pull requests in WP_UnitTestCase_Base::skipOnAutomatedBranches().

Mark the test as failed if the environment variables are unavailable.

Fixes #49050.

#5 @SergeyBiryukov
5 weeks ago

  • Keywords fixed-major added
  • Milestone changed from 5.4 to 5.3.3
  • Resolution fixed deleted
  • Status changed from closed to reopened

[46999], [47000], and [47001] should be backported to the 5.3 branch.

Last edited 5 weeks ago by SergeyBiryukov (previous) (diff)

#6 @SergeyBiryukov
5 weeks ago

In 47001:

Tests: Don't fail the test in WP_UnitTestCase_Base::skipOnAutomatedBranches() if Travis environment variables are unavailable, it prevents from running the test locally.

Follow-up to [47000].

See #49050.

Note: See TracTickets for help on using tickets.