Begin Travis CI testing

[karmatosed]

As brought up in https://github.com/WordPress/twentysixteen/pull/23

[ihorvorotnov]

Voting +1 for Travis.

[philiparthurmoore]

In preparation for discussion, I wanted to put some notes down about how I do this. Some of this may or may not be applicable to the WordPress project, but I think it would help discussion and any confusion for people who are not familiar with Travis CI.

As an example, I'll use Subtitles, since I develop it exclusively on GitHub but release it on WordPress.org. GitHub for Twenty Sixteen is an amazing platform for collaboration (kudos!), and as such it would be amazing to see it used in as many ways possible to aid that collaboration.

Here's how I do it with Subtitles.

The Travis CI / coding standards files that I use are here:

• https://github.com/philiparthurmoore/Subtitles/blob/master/.magnum.yml (Magnum CI)
• https://github.com/philiparthurmoore/Subtitles/blob/master/.travis.yml (Travis CI)
• https://github.com/philiparthurmoore/Subtitles/blob/master/codesniffer.ruleset.xml (Codesniffer rules)

To make sure that these are not released when I run git archive, I have made sure to make note of them in a .gitattributes file:

• https://github.com/philiparthurmoore/Subtitles/blob/master/.gitattributes
• https://github.com/philiparthurmoore/Subtitles/blob/master/.gitignore

On the WordPress.org side of things, the only thing I ever really worry about is excluding all of the GitHub specific files via svn:ignore:

So when you look at the WordPress.org repo all you see are the files that are needed: https://plugins.trac.wordpress.org/browser/subtitles/trunk

The goal here is to avoid pull requests that mess up Twenty Sixteen. A lot of PRs are already coming in, and without automatic testing to make sure that the code stays healthy with regard to coding standards there's a chance with every PR for regressions.

Implementation is very easy; getting this in place now will create a solid foundation upon which future PRs and future 2016 development benefit.

In case I fall asleep in Hanoi during the meeting, this is what I would have said. :)

[westonruter]

:+1: for Travis checks.

I've added comments on the PR for improvements to the PHPCS check, as well as adding JSHint and JSCS checks.

[philiparthurmoore]

I've added comments on the PR for improvements to the PHPCS check, as well as adding JSHint and JSCS checks.

Saw that. Thanks so much for your comments @westonruter. Great stuff.

[karmatosed]

I'm going to be super transparent here with my feelings. I think this test is overkill for the default theme. I totally understand in theory what it is fixing, but it's not something I use in my own theme development or feel comfortable working with. I'm cautious to add yet another hurdle to a repo. I'm very cautious to have something in this which I do not understand every line of.

In saying the above, I recognise the importance of testing, I just do not feel it's needed for this project. I feel it's spiralling in what we're adding also. I'm very nervous of us adding something like this. I will note, I also feel this about _s. I do not believe it has to be part of every theme's development process. We all have our own process.

Ultimately, this decision is up to those of us committing and merging. This is my vote, to not have this in. This will be discussed in the chat this week on Friday. I may not be there, so adding my vote right here. I am totally fine with a consensus of the team on this one.

[philiparthurmoore]

I think this test is overkill for the default theme.

This same method of testing has been used in previous default themes, but manually.

I totally understand in theory what it is fixing, but it's not something I use in my own theme development or feel comfortable working with.

I don't find this a compelling enough reason to avoid a tool that will massively benefit the theme. It's literally two files in the repo, and we're done. It takes the work out of manually checking every single PR for syntax errors, regressions, and standards errors.

I'm cautious to add yet another hurdle to a repo. I'm very cautious to have something in this which I do not understand every line of.

Every line in the PR is commented with an explanation of what's happening. The same can be found in WordPress development repo and the BuddyPress repo.

In saying the above, I recognise the importance of testing, I just do not feel it's needed for this project. I feel it's spiralling in what we're adding also. I'm very nervous of us adding something like this. I will note, I also feel this about _s. I do not believe it has to be part of every theme's development process. We all have our own process.

Yes. Everyone has their own process and this protects the project against that.

Ultimately, this decision is up to those of us committing and merging. This is my vote, to not have this in. This will be discussed in the chat this week on Friday. I may not be there, so adding my vote right here. I am totally fine with a consensus of the team on this one.

I do hope whoever is voting on this will consider how easy and beneficial this will be. It's 2 files, none of which will be released with the theme in Core, and it will catch a lot of what our eyes simply can't.

[ihorvorotnov]

Yes. Everyone has their own process and this protects the project against that.

Exactly. The Great Equalizer)

[westonruter]

Ultimately, this decision is up to those of us committing and merging.

A major point of having these automated code quality checks is to make the job of the committer/merger easier :smile: They can reduce a ton of manual back-and-forth with the contributor and avoids the reviewer being the “bad guy” since the contributor can just shake their fist at Travis instead.

[ihorvorotnov]

Here's a brief discussion during today's meeting, https://wordpress.slack.com/archives/core-themes/p1441383705000303

@philiparthurmoore @westonruter Seems like we need to add some screens and examples of Travis CI usage. I'll try to take care of this tonight. You can add anything that will make the Travis CI adoption easier.

[karmatosed]

This came up in the chat and what would be great to know is the following:

• What exactly line by line is it testing? We should be getting an agreement on that not just accepting a file or even what it has in.
• What process does it do? Travis isn't known by everyone so being clear in a non-technical word is great.
• What happens when things break and how can anyone involved fix this?

We don't need examples where it's worked or been used. It's being used, that's great. This is about understanding and making sure that all committing to this project know whats in any file.

[mendezcode]

What exactly line by line is it testing? We should be getting an agreement on that not just accepting a file or even what it has in.

The lines in the .travis.yml config file do not test anything. They just tell which PHP version to run, which commands to run to prepare the testing environment, and which commands to run to perform the actual tests.

It's similar to a task runner in essence, just like Makefile or Grunt/Gulp, but for running tests. These tests can be used for quality control before merging a Pull Request, for example.

What process does it do? Travis isn't known by everyone so being clear in a non-technical word is great.

Travis CI sets up a Virtual Machine running Linux, and it parses the YML configuration file to prepare the environment. Once everything is configured, the tests are run and the VM is destroyed.

What happens when things break and how can anyone involved fix this?

On Travis, things are not likely to break unless the service is broken. If something breaks in the tests themselves, then anybody with tests knowledge can fix any issue brought up by the tests. This would basically involve fixing the tests themselves.

We don't need examples where it's worked or been used. It's being used, that's great. This is about understanding and making sure that all committing to this project know whats in any file.

Concerning the file, it's just a configuration file similar to .gitignore with the difference that it's committed into the repo since it's needed for the integration with Travis CI. Such file is read by the Travis CI instance on the VM, which determines the behavior of how the enviornment will be prepared and which script runs the test.

[ihorvorotnov]

@karmatosed That's the case when a picture is worth a thousand words. Just to make that black box less of a black :) Or even a screencast, as suggested by @philiparthurmoore on Slack, will guide you (and anyone new to Travis) through this.

About the checks and that unknown travis.yml file. What it does, briefly:

• sets up the testing environment
• installs WordPress and the theme
• installs checks php-codesniffer and wordpress-coding-standards
• runs that test site through these checks (first one checks for PHP errors, second for conforming WP coding standards)

If all checks passed, GitHub will mark the PR green and say it's safe to merge. If any check fails, it will mark it as failed with details - what actually is wrong. The contributor (PR author) will also receive a message with that details so it's easy for him/her to fix the error in his/her code and re-submit the PR.

Everything CI-related happens externally, you don't have to worry about anything. Think of it as a remote virtual server which runs defined automatic checks. Kind of a spell-checker. There's nothing to learn, install, configure or do for anyone contributing to the repo. There's nothing to learn, install, configure or do for anyone reviewing PRs. It just works. If a PR is green, checks passed - review the changes introduced in the PR and accept it. If it says there's a failed check - read the description and see what's wrong. It can be a syntax error in the code, wrong use of spaces instead of tabs etc.

Hope this helps.

UPDATE: @mendezcode was a bit faster than I :)

[philiparthurmoore]

I’ve uploaded a screencast (same video, two links) to explain this more clearly:

• https://cloudup.com/cVLFRavA6na
• https://youtu.be/MFamhbbvDKo

Feedback welcome.

[retlehs]

@philiparthurmoore :+1: great video, you did a very good job explaining everything. thanks for taking the time to make it.

implementing travis & having automated tests is a necessity for twentysixteen considering the number of pull requests that'll be coming in. they're already piling up.

[kalenjohnson]

Good work @philiparthurmoore , and also a big +1 or however many votes I can give for automated testing.

It makes life so much easier when someone submits a PR of any size and instead of reading every. single. update. and pointing out each coding style issue, having Codesniffer tell you the issues. You also look less like a bad guy and just say "please fix the issues caught by Travis before I can review". :joy:

[ihorvorotnov]

That's a great in-depth review, perfect explanation why we really need it! :+1:

However, one of the important concerns was if using Travis CI adds any complications or extra steps to follow for contributors. Not all of us have solid experience with continuous integration and testing. While a maintainer can merge PRs with failed tests, usually it's better to ask contributors to fix the errors and commit changes (well, actually it's even better if contributor himself checks the test results and see if he needs to fix anything).

So I've put some screenshots to show it from contributor's perspective. I took a random PR with failed test from _s repo:

• Here's a PR with clear indication that there's a failed test. Click on "Details" to go to Travis CI:

• Click a failed build job to see the log:

• Read through the log to see the error(s):

• Now you know exactly what file contains an error, on which line and what the actual error is. Fix it, push a new commit and you're done. Now you saved some time and headache for project maintainers and made the world a bit better :-)

[karmatosed]

Thanks a lot for that comprehensive video @philiparthurmoore and taking that considerable time to make it.

I'm not totally sold we're in a place because of not having testing or that we're going to get into a bad one. That said, it being explained helps us all get into a place where we can see the benefit. The degree of that is different depending on view point. There's a lot of person preference going on.

It's worth noting, we do not have the amount of PR just because of not having testing @retlehs. We're slowly working through things in a methodical manner.

• What happens with the PHP and JS issues you pointed out? Can you make a ticket for those right off? I'm thinking collating in one could be good.
• Whose account should this go on? Should we be putting on same as core uses? Does that even matter?
• Depending on agreement of people, lets start working out what it should be testing then. This will probably have opinions that need to be considered.
• Looking back on this ticket, we need to plan the org side and getting these tests ignored. This point is more of a note.

[nathanielks]

I'm not totally sold we're in a place because of not having testing or that we're going to get into a bad one.

Testing is kind of like engine coolant. Will your car run without it? Sure, absolutely. If you run your car too hard, though, your engine is going to be in a world of hurt. Accepting pull requests without automated tests on a project being used by 1^n people is like running your car without coolant.

Hopefully this expresses how valuable testing can be on such a project.

[ihorvorotnov]

Depending on agreement of people, lets start working out what it should be testing then. This will probably have opinions that need to be considered.

IMO it's good to go with the tests showed by Philip in the screencast. These are just basic 'must-have' tests for code errors and coding standards.

[iamtakashi]

Thanks all, specially @philiparthurmoore, @mendezcode and @ihorvorotnov. I'm sold on this.

I'll vote "yes".

[ntwb]

What happens with the PHP and JS issues you pointed out? Can you make a ticket for those right off? I'm thinking collating in one could be good.

This makes sense once the configuration of the proposed Travis CI configuration is finalised in https://github.com/WordPress/twentysixteen/pull/23

Whose account should this go on? Should we be putting on same as core uses? Does that even matter?

This does not matter :)

Depending on agreement of people, lets start working out what it should be testing then. This will probably have opinions that need to be considered.

The comments and proposed changes by @aaronjorbin and @westonruter on https://github.com/WordPress/twentysixteen/pull/23 is the best start, they've covered pretty much everything, I think :smirk:

Looking back on this ticket, we need to plan the org side and getting these tests ignored. This point is more of a note.

I'm not sure who is performing the GitHub to WordPress.org/themes sync but they should be able to exclude any files needed using svn:ignore properties on the w.org side of things fairly easily.

[philiparthurmoore]

Others have addressed the additional questions about the implementation, so I'd like to reiterate that primary end goal of this discussion that is the only thing, at this point, that needs to be given a Yes or No:

Should automatic pull request and commit testing be added into Twenty Sixteen on GitHub to make the default WordPress theme safer, more standards-compliant, and easier to review?

This is the only question at this point that is up for decision. I hope that we do not get lost in the weeds on the implementation, because the implementation is easy enough to modify to fit our needs once we have decided definitively that Yes, we can automate testing.

If the answer is Yes, then this is what would need to happen, in this order:

1. Someone who has Admin rights on the WordPress GitHub repo (@aaronjorbin fits the bill, I think) would need to go into Travis CI and allow Travis CI to be used on the Twenty Sixteen repo. (It should look like this and involved a quick Turn On toggle.)
2. I update my original PR that takes into account review notes by both @aaronjorbin and @westonruter, OR submit a new Pull Request that takes into their review notes and makes a unified file easier to merge in. The two files that this Pull Request will likely include are .travis.yml and codesniffer.ruleset.xml and will only include the absolute bare minimum scaffold in order to run Twenty Sixteen through the WordPress Coding Standards checks.
3. Development continues as normal on GitHub, until Twenty Sixteen is ready to be merged into Core.

---

When Twenty Sixteen is ready to be merged into Core, one of two options will probably happen, I'm guessing:

1. The theme is committed into Core as a clean commit with no history of what happened on GitHub and .travis.yml, codesniffer.ruleset.xml, and any other GitHub-related filed like .jscsrc or .jshintignore is simply deleted and forgotten. Contributions migrate over to WordPress Trac via the ticket/diff system and automated testing stops there. If this option is chosen, enough automated testing will have taken place to ensure an extremely strong first commit into Core, and can still be run manually or automatically from time to time.
2. The theme development continues on GitHub, and magic is created via svn:ignore to ensure that syncing between the two environments is Okay and automated-files never ever make their way to WordPress.org or to a user's theme install. Another clear example is here.

---

These two points above have no bearing on the decision of whether or not automation on GitHub is a good or bad idea right now. I am trying very hard to demystify this and explain it as clearly and simply as possible to take away some of the fear around this process. If there are any additional questions please ask away and I'll be happy to answer them if my internet in Hanoi decides to cooperate with me. Thanks.

[nathanielks]

:clap: :clap: :clap: :clap: :clap:

[ianstewart]

Removed the discussion label, following the discussion in core-themes just now.

https://wordpress.slack.com/archives/core-themes/p1441641860000034

Not sure if we need a new ticket or someone to just move forward on Philip's Yes-list above.

[ianstewart]

Editing the issue title is easier than a new ticket. :)

[iamtakashi]

@philiparthurmoore Can you move this forward? If you need us to do something, just let me know.

[karmatosed]

We need to also bring in the .org side as this repo syncs daily. Before we do anything @philiparthurmoore lets get the input of @dd32.

[philiparthurmoore]

@philiparthurmoore Can you move this forward? If you need us to do something, just let me know.

Yes, absolutely.

We need to also bring in the .org side as this repo syncs daily.

Where does this repo sync daily to? Can you give me the direct URL? I'm not seeing any traces of this theme in Trunk yet so I'm having a hard time understanding how this is related right now: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes

Thanks for taking the time to discuss this during the weekly meeting gang.

[karmatosed]

@philiparthurmoore we sync to the .org theme repo not core. I've pinged @dd32 above as he has set this up. We need to not sync over the travis files.

Note, we should not merge or do any commits until we have this set up.

[philiparthurmoore]

Oh, this URL? https://themes.trac.wordpress.org/browser/twentysixteen/

@dd32, can you make sure that the following files, at a minimum, never make their way to that repo?

.jscsrc
.jshintignore
.jshintrc
.travis.yml
codesniffer.ruleset.xml

Thank you.

[dd32]

@dd32, can you make sure that the following files, at a minimum, never make their way to that repo?

Can we please just leave them in the package? Excluding them adds no significant benefit.

[philiparthurmoore]

Makes absolutely no difference to me, or a theme user for that matter. I'm trying to follow the rules so there's this and the issue of you (or someone with Admin over the repo) toggling Travis CI on so we can proceed. Thanks so much for your timely response. On Sep 8, 2015 12:17 PM, "Dion Hulse" notifications@github.com wrote:

@dd32 https://github.com/dd32, can you make sure that the following files, at a minimum, never make their way to that repo?

Can we please just leave them in the package? Excluding them adds no significant benefit.

— Reply to this email directly or view it on GitHub https://github.com/WordPress/twentysixteen/issues/49#issuecomment-138436599 .

[dd32]

@Otto42 I believe you're an owner who can toggle this stuff? Any chance you're able to help the team get it setup?

[karmatosed]

I've been having a think about this. I 'think' the files are ok to have in the .org repo. But, I've asked for a second opinion as we usually ask for any files not used to be cleared up. That's what my gut reaction was to ask them to be removed. The usual ruling is 'unless used removed'. As with all admin decisions, we check with someone so if we can hold off until we get a ruling please.

[philiparthurmoore]

As with all admin decisions, we check with someone so if we can hold off until we get a ruling please.

Who is making what ruling, and about what? Can you clarify this? Thank you very much!

[dd32]

Regardless of any repo guidelines, I'd like to keep the zips consistent for a simpler sync script, removing the version bumps from the sync and instead having them happen via Github is going to make that vastly easier.

If absolutely required, and I'm yet to see any good reason to, the files can be excluded, but probably not until end of next week.

[karmatosed]

All I have asked is for another theme review admin to give their interpretation of if the files have to be removed. Let's take it from there step by step without assumption.

[philiparthurmoore]

All I have asked is for another theme review admin to give their interpretation of if the files have to be removed.

Thanks for explaining that; very helpful information. It wasn't very clear to me.

[grappler]

As the whole process of syncing the GitHub repo to the theme directory I feel that leaving the files will not cause any issue. The files will be removed anyway in the final version.

@dd32 How does the sync script work? Could @philiparthurmoore suggestion of using .gitattributes work?

To make sure that these are not released when I run git archive, I have made sure to make note of them in a .gitattributes file:

[karmatosed]

After chatting to José, I feel safer in saying let's keep the files in there on the.org theme repo. I wanted to be sure as this is obviously the easier option.

[philiparthurmoore]

Thanks. I'll make a fresh PR tomorrow.

[Otto42]

Can somebody lay out what needs to be done, like, really specifically? I'm happy to make any admin side/owner changes needed, as long as you all agree, but I don't use Travis myself and know nothing about it.

[Otto42]

Also, would we need some kind of WordPress organizational account on Travis? I can set that up too, but I think we need a plan here, to avoid tying it to Automattic or similar. Happy to do it, totes can, just need info on the best/right way.

[philiparthurmoore]

Hey Otto. It's simply a matter of hitting a toggle. Travis already knows what repositories are on GitHub.

1. Go to https://travis-ci.org/
2. Authenticate your account with GitHub
3. Go to https://travis-ci.org/profile/yourgithubusername
4. Turn the toggle on for Travis CI for the WordPress/TwentySixteen repo

That's the only thing we'd need right now. Thanks.

[philiparthurmoore]

Example: https://travis-ci.org/Automattic/_s

The 2016 repo URL will be https://travis-ci.org/WordPress/twentysixteen

Anyone with Admin rights over this 2016 repo in github can do this. It's a matter of letting the repo know on GitHub that it's okay to give Travis CI access to its commits and PRs

[Otto42]

Okay, that's done.

https://travis-ci.org/WordPress/twentysixteen

The rest is on you. If you need me to change something, lemme know.

[philiparthurmoore]

Easy peasy. Thank you very much for taking care of this.

[karmatosed]

I kind of agree with @Otto42 about a non tied account. Does anyone know how we can get that setup? It would help in the future potentially.

[ntwb]

I kind of agree with @Otto42 about a non tied account. Does anyone know how we can get that setup? It would help in the future potentially.

It's not needed, Travis CI is Free as in :beer: for open source projects, it's only linked to the repo parent organization so that some basic permissions and access can be controlled by the repo owner and not forks of the main repo etc

[philiparthurmoore]

Travis CI is just a service on this specific repo.

If for whatever reason Otto leaves WP in the next few weeks (the likelihood of which is around the same as me learning Russian by tomorrow) and the connection to Travis CI is lost, then someone else with repo Settings power will just need to reconnect it, which takes seconds.