Run integration tests for PRs that change test/integration
#9807
Conversation
|
/to @erwinmombay, who may know why we don't run integration tests for PRs today. |
|
They take forever, and it has (had?) the potential to leak our SauceLabs tokens. |
|
The "forever" part is significantly better now than it used to be, with sharding, faster builds, etc. Here's an estimate on how much wall clock time this will add to a single PR round-trip: From looking at the builds on https://travis-ci.org/ampproject/amphtml/pull_requests, a typical PR run that includes unit tests is split across two shards. The unit_tests shard takes about 15-20 mins, while the integration_tests shard returns almost immediately. With this change, both shards will take about 15-20 minutes, so I expect a minimal effect on wall clock time. As for the concern about slowing down the whole Travis queue, I expect that only a small percentage of PRs actually touch I'll dig into the token secrecy to see if that's still an issue we should be concerned about. |
|
I think this relationship is unidirectional. Changes to almost anything in |
|
@choumx, I agree that that's what we should be doing. However, running them for all PRs will double the load on the Travis PR queue as things stand today. I figured this PR would alleviate the pain for those who are trying to fix master, without making a huge change to the Travis load. |
|
@erwinmombay, this cannot be merged until the following things happen:
Will hold off until then. |
|
Those should be fixed now. |
|
Merging this so I can make master green. YOLO. |
|
@jridgewell, this will block everyone else from merging PRs unrelated to the redness on master. Let me know if I can help with the master-greenification effort. |
* Revert "Add a caches.json file that provides meta data about AMP Caches. (#9804)" This reverts commit 55a3ac0. * Revert "Fix flaky amp-animation test (#9817)" This reverts commit 4a9cf91. * Revert "Run integration tests for PRs that change `test/integration` (#9807)" This reverts commit 233c76e. * Revert "Finally, fix visState integration tests (#9806)" This reverts commit 47f9191. * Revert "add jwt plugin (#8672)" This reverts commit a23cbd1. * Revert "Add missing methods to AmpContext tests (#9682)" This reverts commit 646f103.
If you have to edit an integration test today, the workflow is to test your changes locally, run the PR through Travis (where integration tests are not run), and then merge it to master (where integration tests are run on all platforms), and then hope they pass.
This PR will run integration tests as part of the PR check for only those PRs that touch
test/integration. While it may slightly slow down Travis runs for those PRs, it gives you test results before your changes hitmaster.Looking at the history of
test/integration, it appears that we only have a small handful of changes per week going into this directory, so I don't expect it to have a huge impact on the Travis queue.A good compromise, methinks.