Document Contribution Instructions

[ericmann]

The unit test suite that comes with the plugin isn't functional out of the box as it hinges on S3 credentials either set in constants or pulled through getenv(). This works on Jenkins as the proper environment variables are set, but not locally without help.

Using the WP-CLI unit test scaffold is great for getting started, but if Jenkins is going to be a bottleneck for contributions, then all contributors should be able to easily get tests running locally. Ideally, a quick-and-dirty setup would be documented in CONTRIBUTING.md for future devs.

[ericmann]

Note, this is also an issue with contributing pull requests as, for some reason, the S3 credentials are set on Travis when some people push their code but not for everyone.

For example, https://travis-ci.org/humanmade/S3-Uploads/jobs/68606445 pushed by Joe:

Setting environment variables from repository settings
$ export S3_UPLOADS_BUCKET=[secure]
$ export S3_UPLOADS_KEY=[secure]
$ export S3_UPLOADS_SECRET=[secure]

However, when I push my PR, these constants are not set and PHPUnit experiences a fatal error during execution:

PHP Fatal error:  Class 'S3_Uploads_Image_Editor_Imagick' not found in /home/travis/build/humanmade/S3-Uploads/tests/test-s3-uploads-image-editor-imagick.php on line 46

See https://travis-ci.org/humanmade/S3-Uploads/jobs/68874617 for the entire console output ...

[joehoyle]

Yes, this is a tricky one. I'm using TravisCI secret env vars to pass the s3 keys, as I can't publish those. I think maybe documentation on how to use your own AWS credentials locally would be good, but I don't know of a way to securely run travis ci tests on 3rd party repos.

[ericmann]

The weird thing is that Travis is running CI tests against the humanmade repo, but is checking out the PR as a branch before it runs the tests. So, conceptually, the same secret ENV variables should work on Travis ... they just aren't being loaded.

[joehoyle]

@ericmann travis intentionally doesn't include env variables on 3rd party repos for security reasons. If you (or anyone) wanted, they could write so code to expose the secrets in the travis log and get them, hence only including env vars on the humanmade org origin

[ericmann]

I understand that. My point is that Travis is already running everything from the HumanMade repo, not from my fork.

$ git clone --depth=50 git://github.com/humanmade/S3-Uploads.git humanmade/S3-Uploads
Cloning into 'humanmade/S3-Uploads'...
remote: Counting objects: 2829, done.
remote: Compressing objects: 100% (1127/1127), done.
remote: Total 2829 (delta 1602), reused 2806 (delta 1583), pack-reused 0
Receiving objects: 100% (2829/2829), 1.78 MiB | 0 bytes/s, done.
Resolving deltas: 100% (1602/1602), done.
Checking connectivity... done.
$ cd humanmade/S3-Uploads
0.14s$ git fetch origin +refs/pull/28/merge:
remote: Counting objects: 22, done.
remote: Compressing objects: 100% (14/14), done.
remote: Total 22 (delta 15), reused 14 (delta 8), pack-reused 0
Unpacking objects: 100% (22/22), done.
From git://github.com/humanmade/S3-Uploads
 * branch            refs/pull/28/merge -> FETCH_HEAD
$ git checkout -qf FETCH_HEAD

Setting environment variables from .travis.yml
$ export WP_VERSION=latest
$ export WP_MULTISITE=1

...

The only thing I could see that would prevent ENV variables from being available here is Travis switching things based on who triggered the test run, in which case having test runs on PRs is kind of pointless since anyone outside of HumanMade will never have a passing set of tests.

[joehoyle]

in which case having test runs on PRs is kind of pointless since anyone outside of HumanMade will never have a passing set of tests.

Why is this pointless? What about PRs from within Human Made?

[ericmann]

Sorry, I came on a bit strong there.

I meant pointless from the perspective of third-party contributions. From the outside looking in, any non-HM pull requests are flagged in red due to failing CI tests. But as they're non-HM pull requests, there's no chance they can pass CI tests in the first place.

So in terms of internal work, yes, they're still useful. In terms of open collaboration with others on GitHub they're just a huge red flag that's not really a useful way to determine the quality/health of a PR.

[joehoyle]

I meant pointless from the perspective of third-party contributions. From the outside looking in, any non-HM pull requests are flagged in red due to failing CI tests. But as they're non-HM pull requests, there's no chance they can pass CI tests in the first place.

My usual process is to git fetch the 3rd party origin, and push it up to to humanmade. As the commits are then in the hmn repo too, travis will expose the env vars to the pull-request opened by the third party - but yeah it currently requires that manual step.

I'd like to not do any of this of course, but I don't know of a better way to achieve it.

[spacedmonkey]

For what it's worth, I agree, unit test should have third party decencies like Apis. Anything api call should be mocked. How about (just for the travis run of the test) you use something like this - https://github.com/jubos/fake-s3 not used it personally but heard good things...

[joehoyle]

@spacedmonkey unit tests typically not - but most of our tests are integration tests, it's specifically testing the integration with s3, weird s3 behaviours n all, mocking that would defeat the purpose of most of what we are trying to test.

I'm not saying there would be no use for mocking, just that realistically we still need the integration test, if the goal is to not let bugs creep in.

I'm thinking I could bundle some keys that only have access to read/write to the specific location and have something that periodically deletes everything at that location, so they can be treated as public.

[spacedmonkey]

What I was saying was that, you could just require fake s3 using composer on dev. You only need to do the mocking for the travis builds. It means the build status will work on travis and you can still run the test where ever to get real integration data.

So basically, travis - unit test, everywhere else they are integration tests.

[joehoyle]

yeah, no don't like that :P I want travis builds to be the main source of pass/fail for testing. I think there are better ways to solve, given the primary goal is to have tests fail.

I'll add a section to the readme on testing to describe how people can test using their own credentials locally, and see if we can have the travis ones public (per my previous comment).