Refactor visual diff tests, and return build status from pr-check.js #11196
Conversation
rsimha
changed the title
|
/to @cramforce @choumx |
build-system/tasks/visual-diff.rb
Outdated
| def waitForBuildCompletion(buildId) | ||
| tries = 0 | ||
| until ['finished', 'failed'].include?(getBuildStatus(buildId)['state']) | ||
| sleep(5) |
There was a problem hiding this comment.
Make 5 a constant, since you refer to it below
build-system/tasks/visual-diff.rb
Outdated
| # Args: | ||
| # - buildId: ID of the Percy build. | ||
| def verifyBuildStatus(buildId) | ||
| status = getBuildStatus(buildId) |
There was a problem hiding this comment.
Since you always call waitForBuildCompletion first, maybe pass in the status that the wait process yielded?
build-system/tasks/visual-diff.rb
Outdated
| visualTestsConfig = JSON.parse(visualTestsConfigJson) | ||
| buildId = runVisualTests(visualTestsConfig) | ||
| waitForBuildCompletion(buildId) | ||
| verifyBuildStatus(buildId) |
There was a problem hiding this comment.
Would it be possible to split this into two build steps?
- Start visual diff as early as possible in build
- At the end run only the polling after other build steps completed, so that the polling time is minimized.
There was a problem hiding this comment.
Done. I've optimized the number of build steps between creating and verifying the visual diffs, so that we still fail fast if there are diffs, and yet, don't have to wait around for any amount of time.
|
@cramforce, PTAL. |
rsimha
dismissed
cramforce’s stale review
All feedback addressed. PR approved by @choumx.
| @@ -326,6 +329,7 @@ function runAllCommands() { | |||
| command.runVisualDiffTests(/* opt_mode */ 'master'); | |||
| command.runJsonAndLintChecks(); | |||
| command.runDepAndTypeChecks(); | |||
| command.verifyVisualDiffTests(); | |||
There was a problem hiding this comment.
Don't you want to do this after runUnitTests?
There was a problem hiding this comment.
No, this was intentional, because by the time runJsonAndLintChecks and runDepAndTypeChecks are run, we can be sure that the visual build has been processed. This way, we can fail early if there are visual diffs, instead of waiting for the unit tests to run.
Let me know if you'd prefer that the check be moved to the end.
There was a problem hiding this comment.
Here's an example of the timing I describe above: https://travis-ci.org/ampproject/amphtml/jobs/273041116
There was a problem hiding this comment.
I think I'd prefer to do it later. I can see that we'll add so many v-diff tests that they will go much slower.
There was a problem hiding this comment.
SGTM. Just sent you #11211 with the change.
Fixes #10536