Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Travis and Unit test updates #136

Closed
wants to merge 13 commits into from
Closed

Travis and Unit test updates #136

wants to merge 13 commits into from

Conversation

JPry
Copy link
Contributor

@JPry JPry commented May 8, 2018

This update aims to make it easier to run unit tests locally, while also making some updates to Travis. In particular:

  1. We're now using the Travis Build Matrix more effectively.
  2. We're not testing against the latest version of WordPress (4.9 at the time of this PR), as well as testing against WordPress trunk. Additionally, any test failures against WordPress trunk will not trigger a failed build.
  3. Trusty is used with Travis for all version of PHP 5.4 and later. 5.3 is tested against Precise
  4. Code coverage is only run when we've actually generated the clover.xml file. This prevents needlessly running codecov.io.
  5. PHPUnit is now included with Composer, which means we don't have to overwrite the version provided by Travis, and we're able to get the same results locally
  6. There's a dedicated run_tests.sh script to run tests, whether locally or on Travis.
  7. The bootstrap.php file does a better job of determining where the WordPress tests directory lives.

@JPry JPry added type: enhancement The issue is a request for an enhancement. needs tests The issue/PR needs tests before it can move forward. labels May 8, 2018
@JPry JPry requested a review from thenbrent May 8, 2018 20:09
@thenbrent
Copy link
Contributor

thenbrent commented May 9, 2018 •
edited by JPry
Loading

Lots of amazing additions here @JPry!

The README on setup/running locally needs work though. Ideally, it should be sufficiently comprehensive that it can get anyone running tests, regardless of their existing development environment. At the moment, it assumes too much of the reader and their local environment.

For example, it looks like local.sh will create a test database named wordpress_test (that's not mentioned in the README) and running that command requires mysql to be installed and accessible via bash with a root user without a password.

The composer dependencies also require PHP 7.1: https://cl.ly/1W162Z1O233q

Those are requirements/pre-requisites that should be explicitly mentioned, and ideally accounted for with instructions for how to set them up.

The README does say:

This can be configured easily using a tool such as Vagrant (and VVV), or using another solution of your choice for locally running a WordPress site.

But that's too brief to help a reader who isn't already familiar with VVV and who isn't familiar enough with the internals of their local setup to find and meet the requirements of the test suite.

To overcome the many possible local variants (it's impossible to write a guide to handle them all!), it's OK to be opinionated. Provide clear instructions or complete instructions on how to run the test suite with one tool, like VVV, or Chasis or Valet. That makes it easy for someone with the wrong environment to still run the tests, and those who already know how to configure their environment can ignore those instructions/opinions (but will likely still glean valuable tips from them, like PHP version requirements etc).

For an example of the type of detail I think should be included, see the WooCommerce Subscriptions Unit Test README.

@JPry
Copy link
Contributor Author

JPry commented May 9, 2018

@thenbrent I've added a first round of updates to the Readme file based on your suggestions. I used VVV as the tool of choice since that's what I'm most familiar with. It's not quite as verbose as the readme for subscriptions that you linked to, but I think it's closer to what we're looking for.

The composer dependencies also require PHP 7.1: https://cl.ly/1W162Z1O233q

This should no longer be the case. Apparently, when composer update is run with PHP 7.1 (or greater), the dependencies that are pulled in could potentially be higher than what we really need. I ran composer update again using PHP 7.0, and rebased the changes into the commit history. I've also mentioned the need for PHP 7.0+ in the readme.

Let me know what other changes you think are necessary here, and I'll be glad to make additional updates.

@JPry
Copy link
Contributor Author

JPry commented May 11, 2018

I noticed that this had merge conflicts with composer.json (thanks to #137). I've rebased against master and the merge conflicts are now fixed.

@JPry
Copy link
Contributor Author

JPry commented May 31, 2018

@thenbrent This has been rebased again with the latest composer.json changes

JPry added 13 commits June 18, 2018 14:23
@JPry
Initial updates to .travis.yml
43bd2ad 
- Update matrix, and include WP trunk
- Test PHP 5.3 on WP 4.9

It seems better to use the native matrix as much as possible with exclusion rather than adding inclusions to the matrix manually
@JPry
Use Git to clone appropriate version of WordPress
71ccada
@JPry
Update format of variables in setup.sh file
6a4d71b
@JPry
Move travis/setup.sh into dedicated setup directory
95e66f2 
Also:
- Use uppercase for variables. This is the normal convention in shell scripts
- Use quotes around variables
- Ensure we have the WP_VERSION variable available. This is just extra insurance. It probably will never be undefined
- Update author info
- Only create DB if it doesn't exist
@JPry
Update composer to include phpunit
6d16f92
@JPry
Clean up bootstrap file
e547a23
@JPry
Create a local install script
d658cbf
@JPry
Create dedicated travis config file
c390056
@JPry
Create dedicated local config
1a9ae10
@JPry
Create dedicated script for running tests
fb4b923
@JPry
Additional updates to .travis.yml
9466959 
- Ensure composer dependencies are installed
- Ignore platform requirements with composer
- Define latest version of WP
@JPry
Run appropriate phpunit depending on PHP version
95a5f81 
Also:
- Move composer install to travis script
- Only run code coverage when needed, and only after a successful run
- Cache some composer files
- Update script for running tests
@JPry
Add readme for tests
3223adc
@JPry
Copy link
Contributor Author

JPry commented Jun 18, 2018

@thenbrent I'm planning to update with instructions for Valet+, for a couple reasons:

  1. I've stopped using VVV/Vagrant for my own local development
  2. Valet instructions are already in use for Subscriptions.
  3. Because of the above point, I should have just gone that route to begin with 😬

@thenbrent thenbrent added this to the 2.1.0 milestone Jul 26, 2018
@thenbrent
Copy link
Contributor

I'm planning to update with instructions for Valet+

@JPry do you still plan to do that? I'd personally vote for that over VVV. 👍

@JPry
Copy link
Contributor Author

JPry commented Aug 20, 2018

@thenbrent Yep, that's still in the plan. I have this one on my list for the week.

@JPry JPry added the status: in progress This is being worked on. label Aug 20, 2018
@JPry JPry removed this from the 2.1.0 milestone Sep 17, 2018
@JPry JPry added this to the 2.2.0 milestone Sep 17, 2018
@thenbrent thenbrent changed the base branch from master to version_3_0_0 July 11, 2019 03:10
@thenbrent thenbrent modified the milestones: 2.2.0, 3.0.0, 3.1.0 Jul 23, 2019
@rrennick rrennick modified the milestones: 3.1.0, 3.2.0 Feb 21, 2020
@rrennick
Copy link
Collaborator

rrennick commented Jun 3, 2020

@JPry This one is quite stale so I am closing it.

@rrennick rrennick closed this Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thenbrent thenbrent Awaiting requested review from thenbrent

Assignees
No one assigned
Labels
needs tests The issue/PR needs tests before it can move forward. status: in progress This is being worked on. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@JPry @thenbrent @rrennick