Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#5708 closed defect (bug) (fixed)

Broken Travis CI PHP 5.2 tests and improved Travis CI performance

Reported by: netweb Owned by:
Milestone: 2.1 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

Currently Travis CI PHP 5.2 tests are failing, though it is not reporting as failed. The Travis CI PHP 5.2 environment cannot locate the wp-tests-config.php file.

Example https://travis-ci.org/buddypress/BuddyPress/jobs/27356614

The attached patch fixes this and also improves Travis CI performance and reporting

Comparrison of builds via latest r8503 changeset
https://travis-ci.org/buddypress/BuddyPress/builds/27356612
https://travis-ci.org/ntwb/BuddyPress/builds/27369740
(By coincidence the build above includes two errored builds where Travis failed to install the NPM dependencies)


  • Switch to a 'shallow' Git clone of the official WordPress Git repo for faster WordPress upstream checkout, timings below
  • The addition of the fast_finish: true flag in the build matrix will mark the build passed or errored as soon as a single build job has failed. Previously the build status was not reported until ALL tests had completed regardless of status.
  • Moves the 'build configuration' to before_install allowing non-BuddyPress test configuration to report as errored (MySQL database creation, and WordPress Git checkout etc). When a failure occurs in this section the build is marked as errored which is a more accurate description in that the environment we are setting up has failed and not the actual BuddyPress tests we are testing.
  • Introduces HHVM testing into the build matrix and allowed to fail in that they will not be included in the overall build job failed, errored, or passed build status. This allows BuddyPress testing to play nice with alpha/beta versions of HHVM until stable and officially released.
  • Changes the PHP 5.5 test environment to disable WP_DEBUG with WordPress 3.7 branch, 3.8, 3.9 and 4.0 are not affected by the mysqli issue.

Attachments (1)

5708.patch (4.4 KB) - added by netweb 5 years ago.

Download all attachments as: .zip

Change History (15)

@netweb
5 years ago

#1 @DJPaul
5 years ago

Cool. Questions:

  • What is getenv( 'TRAVIS' )?
  • Does WP's own unit tests pass with HHVM? I thought about adding HHVM last time I touched the file but on reflection I decided until we *know* WordPress itself works properly on HHVM, it wasn't worth us testing against.
  • Similarily, if we decide to test against HHVM and there are errors, do we care enough about HHVM to fix them?
    • If we do, we should make a plan to make that happen, and have someone responsible for fixes.
    • If we don't, what's the point running the tests? If someone wanted to start making it work, they could run the tests in their local HHVM-enabled environment.

We should probably also wait until the current issue with LIKE_ESCAPE is resolved so we have passing tests before we apply any of these proposed changes to the config. :) https://core.trac.wordpress.org/ticket/10041

#2 follow-up: @boonebgorges
5 years ago

Thanks, netweb.

  • Change to git clones seems good to me
  • fast_finish seems fine
  • before_install seems good

Questions:

  • Can you clarify why the Travis-specific paths are required in define-constants.php?
  • +1 on DJPaul's HHVM questions
  • Can you expand on the WP 3.7 point? I don't follow, and I don't see broken builds on 3.7.
  • I can't see at a glance what it is in your patch that's fixing the builds for PHP 5.2. Can you clarify?

#3 in reply to: ↑ 2 @netweb
5 years ago

Replying to DJPaul:

What is getenv( 'TRAVIS' )?

Travis CI includes a bunch of #Environment-variables that can be used throughout testing scripts.

Does WP's own unit tests pass with HHVM? I thought about adding HHVM last time I touched the file but on reflection I decided until we *know* WordPress itself works properly on HHVM, it wasn't worth us testing against.

No, not currently, HHVM & PHP 5.6 allow to fail testing will be in WordPress Core shortly #WP28301. So in theory once this is included iteration can be done so that the test do start to a) run and b) pass.

Similarily, if we decide to test against HHVM and there are errors, do we care enough about HHVM to fix them?

I would say yes, there has been work to get WordPress running on HHVM and fully expect this to be a supported PHP version in the future.

If we do, we should make a plan to make that happen, and have someone responsible for fixes.
If we don't, what's the point running the tests? If someone wanted to start making it work, they could run the tests in their local HHVM-enabled environment.

I would say because WordPress Core is working on HHVM compatibility we should follow suit, there may not be a detailed roadmap yet so I'd suggest we just roll with it ;)

We should probably also wait until the current issue with LIKE_ESCAPE is resolved so we have passing tests before we apply any of these proposed changes to the config. :) https://core.trac.wordpress.org/ticket/10041

Either/or, none of the changes in this patch change the current tests per se, just speed improvements and the PHP 5.2 fix

Replying to boonebgorges:

  • Change to git clones seems good to me
  • fast_finish seems fine
  • before_install seems good

Cool

Questions:

Can you clarify why the Travis-specific paths are required in define-constants.php?

Because no matter how or what I tried I could not get PHP 5.2 to find the wp-tests-config.php file, I walked up, down, left, right, throughout the directory structure looking for it, never to be found, gave up and used the Travis CI environment variable to just explicitly declare it. I went back through a ton of Travis CI results trying to find the last time PHP 5.2 actually did find the file and again, gave up looking so I have no idea when this became broken.

  • Can you expand on the WP 3.7 point? I don't follow, and I don't see broken builds on 3.7.

The previous config that @DJPaul implemented for disable WP_DEBUG for PHP 5.5 due to ext/mysqli E_DEPRECATED errors was disabling WP_DEBUG for all WordPress versions running under PHP 5.5, we only need to do this for WordPress 3.7.x branch, 3.8.x included a fix for this, and 3.9.x full support for mysqli was added.

  • I can't see at a glance what it is in your patch that's fixing the builds for PHP 5.2. Can you clarify?

The PHP 5.2 tests could not find the wp-tests-config.php file, so the PHPUnit tests would never start, it was lost, but now is found ;)

#4 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to 2.1

#5 follow-up: @DJPaul
5 years ago

Either/or, none of the changes in this patch change the current tests per se, just speed improvements and the PHP 5.2 fix

My concern is that I'd prefer to know our tests are 100% passing before we introduce changes to the test config, in case those changes break something accidentally. We'd see straightaway the regression, where if we commit the change when we know our tests are already broken, we'd be unaware for much longer.

#6 @netweb
5 years ago

I just took another look at the Travis CI builds, the last time a PHP 5.2 Travis CI test coud find the wp-tests-config.php file was build 830 before the switch in r8017 to use develop.svn.wordpress.org.

https://travis-ci.org/buddypress/BuddyPress/builds/19892546 Travis CI Build 831
https://travis-ci.org/buddypress/BuddyPress/builds/19868680 Travis CI Build 830

Last edited 5 years ago by netweb (previous) (diff)

#7 in reply to: ↑ 5 @netweb
5 years ago

Replying to DJPaul:

My concern is that I'd prefer to know our tests are 100% passing before we introduce changes to the test config, in case those changes break something accidentally. We'd see straightaway the regression, where if we commit the change when we know our tests are already broken, we'd be unaware for much longer.

Could split the patch in two, the changes in define-constants.php would get PHP 5.2 working now.

Testing the define-constants.php patch now against r8059 with no changes in .travis.yml appears to get the PHP 5.2 tests running now.

https://travis-ci.org/ntwb/BuddyPress/builds/27538625
https://travis-ci.org/buddypress/BuddyPress/builds/27528336

My suggestion thus would be commit the changes in define-constants.php now and .travis.yml later once BP's PHPUnit are passing.

#8 @boonebgorges
5 years ago

In 8510:

Switch to git checkouts of WP for Travis CI tests

This change results in faster build time

See #5708

Props netweb

#9 @boonebgorges
5 years ago

In 8512:

Fix Travis CI tests for PHP 5.2

PHP 5.2 tests have been broken on Travis CI since r8017, the switch to using
WP's "develop" suite instead of the old wp-tests repo. This changeset fixes
these builds by making the following changes:

  • Better consistency with the use of environment variables (swap out the old WP_CORE_DIR for WP_DEVELOP_DIR, which is what BP itself expects when setting up PHPUnit)
  • Use of export when creating the environment variable, to ensure that PHP has access to it across shell sessions

Hat tip to netweb for an initial patch addressing this issue

See #5708

#10 @boonebgorges
5 years ago

In 8513:

In Travis CI tests, set fast_finish flag to true

This change allows a build to be marked as failed as soon as a single job in
the build matrix has failed, providing faster feedback for devs.

See #5708

Props netweb

#11 @boonebgorges
5 years ago

In 8515:

Only disable WP_DEBUG on PHP 5.5 Travis CI tests when running WP 3.7

This disabling is meant to allow builds to pass even in the presence of mysqli-
related debug notices. But later versions of WP either suppress the notice, or
support mysqli natively.

See #5708

Props netweb

#12 @boonebgorges
5 years ago

In 8516:

In Travis CI config, move environment setup to before_install, so as not to trigger failed builds

Environment setup on Travis CI can fail in a number of ways - failed git
checkouts, database connection problems, etc - that have nothing specifically
to do with BuddyPress's test configuration. This changeset moves those steps
into the before_install section, with the result that a failed setup routine
will result in an 'errored' rather than 'failed' build. Grunt setup and the
phpunit tests themselves will continue to generate failure messages when
something goes wrong.

See #5708

Props netweb

#13 @boonebgorges
5 years ago

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

In the series of changesets above, I've taken the following from netweb's original patch:

  • moved to Git checkouts of WP
  • set fast_finish: true
  • reenabled WP_DEBUG on PHP 5.5 tests when running WP > 3.7
  • moved most environment setup to before_install

I was able to solve the PHP 5.2 issue without resorting to changes in our PHPUnit bootstrap or explicit references to Travis in BP. Specifically, the problem was related to the use of the export command when setting environment variables (it appears that PHP 5.2 was not running in the same shell session as the Travis setup script - maybe it's sandboxed or something).

Regarding HHVM:

there has been work to get WordPress running on HHVM and fully expect this to be a supported PHP version in the future.

You may be right about this, but let's cross this bridge when we come to it. I'd rather not muck about in BP and waste Travis resources running builds that we're not planning to support. When WP announces full support for HHVM, let's reassess.

Thanks again for the initial patch, netweb.

#14 @netweb
5 years ago

Cool, nice, thanks, looks good :)

Note: See TracTickets for help on using tickets.