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

Use local validator binary for tests #10158

Merged
merged 5 commits into from Jun 28, 2017
Merged

Use local validator binary for tests #10158

merged 5 commits into from Jun 28, 2017

Conversation

dreamofabear
Copy link

Fixes #10154.

/to @rsimha-amp /cc @Gregable @cramforce

@dreamofabear dreamofabear requested a review from rsimha June 27, 2017 17:09
dreamofabear
dreamofabear commented Jun 27, 2017
View reviewed changes
.travis.yml Outdated
@@ -52,6 +52,9 @@ matrix:
username: "amphtml"
jwt:
secure: "Wze0F0vGL0UcxryOx1n/vcuD5LIMGyR+69Nc6IWLoRvZBbbIpFwVFhDE6rE9ranIXiA2Hc684N4sV8ASfNDF8RRSB+jyLov159qwgji2rBxIfQ/4kuDV2vYoAJvYMz8m42kwx5FV2VV9awqMMt8mwU3wYIrKIaVCxB34uV86KIlDlbrHxt17Bm5EIiUmwi9r1AAnW/63vVRUN264D77oB4j9UQ759PfD6BDwEt54O87KurNIaLseNCr1IvzfL8veEsZ3uTbLC1GtgHfR4IGgkS2YyN2QIk06VZWeRDEOalS3RcY0nDkbCmBywxIGObnrpEMzOpjBiOb2fxLoLvvpjlla5W84zJGfWE6q4T9IvkyHuDJE+sft5B+arjMIeA6PIeUhKdV27+6qqDEf7fILZ/U/Ekn9ds4zSV8hekAZPUyyPncOeyWppCIJ8sOeCrsebkRjH1BoX/d+FE+nP0bN/XkBpIi/nManx5FyS/kqjQWGKmvsFQfEWlSUaZi7XtEQEjvBizRkzvpJanSDaoiTDS2Keulmwii3XRId51FuGtnfDZFeggLaMTKGfBX9DlPkccwYAZe6vPNfYk1pNgEj6AtnifEhYVEO+aAuWhEnJ86od+1wDOL/h+a2XY6h8/gFBywsD95p7sXPfdVDCKgwagiBo+Hw5MNjztVF7lszg1A="
packages:
- protobuf-compiler
- python-protobuf
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rsimha-amp Should we move sauce_connect and jwt above to the top-level addons to avoid dupe here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explicitly moved the addon to inside the integration_tests shard, since we don't want to unnecessarily do a sauce connect for the other shard, which doesn't run the integration tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, did you mean that there are two addons sections, one at the top level, and one within the shard? If so, that is intentional for the reason I cited above.

I also noticed that you are now calling buildValidator in both shards. If that function relies on the packages you've added here, you'll need to move them to the top-level section.

Edit: From the Travis build, it looks like they are needed in both shards.

rsimha
rsimha reviewed Jun 27, 2017
View reviewed changes
build-system/pr-check.js Outdated
}
if (process.env.BUILD_SHARD == "integration_tests") {
command.cleanBuild();
command.buildRuntimeMinified();
command.runPresubmitTests(); // Needs runtime to be built and served.
command.runIntegrationTests();
command.buildValidator();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By always doing this in both shards (instead of just in the unit_tests shard and only for validator changes), we're adding an extra 2m 30s to the Travis build cycle. I'm guessing there's no way to avoid this, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looks like the test doesn't run on sauce labs anyways (skipSauceLabs()), so we can move it out of the test/integration folder and run it as a unit test instead. Doing that now.

test/integration/test-example-validation.js
if (!window.validatorLoad) {
window.validatorLoad = (function() {
const s = document.createElement('script');
s.src = 'https://cdn.ampproject.org/v0/validator.js';
s.src = '/validator/dist/validator_minified.js'; // Served by app.js.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could've sworn that the google3 style guide requires 2 spaces between ; and // for inline comments, but I can't cite a source. I guess you're off the hook? :)

build-system/pr-check.js Outdated
@@ -376,7 +377,8 @@ function main(argv) {
command.runDepAndTypeChecks();
// Skip unit tests if the PR only contains changes to integration tests.
if (buildTargets.has('RUNTIME')) {
command.runUnitTests();
command.buildValidator();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is run, line 388 will be duplicated work, and will add an extra 2m 30s to each build, increasing the total Travis CPU time by about 5 minutes (see my other comment for line 298). At a minimum, we'd need to change the if case in line 387 to buildTargets.has('VALIDATOR') && !buildTargets.has('RUNTIME')

rsimha
rsimha reviewed Jun 27, 2017
View reviewed changes
build-system/pr-check.js Outdated
@@ -398,7 +400,8 @@ function main(argv) {
util.colors.cyan('test/integration'));
command.cleanBuild();
command.buildRuntimeMinified();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the multi-part review. I wonder if validator_minified.js is generated by buildRuntimeMinified, which calls gulp dist --fortesting. If so, we can avoid the call to buildValidator before calling runIntegrationTests.

@rsimha rsimha mentioned this pull request Jun 27, 2017
Closed
@dreamofabear dreamofabear force-pushed the test-example-validation-hermetic branch from dd27a1e to 8081e54 Compare June 27, 2017 20:43
rsimha
rsimha reviewed Jun 27, 2017
View reviewed changes
build-system/pr-check.js Outdated
@@ -286,7 +286,7 @@ function runAllCommands() {
command.runVisualDiffTests();
command.runJsonAndLintChecks();
command.runDepAndTypeChecks();
command.runUnitTests();
command.runUnitTests(); // Requires validator to be built.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be reverted, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dreamofabear dreamofabear force-pushed the test-example-validation-hermetic branch from c73772c to dcb56ea Compare June 27, 2017 20:48
rsimha
rsimha approved these changes Jun 27, 2017
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, much simpler and cleaner :)

@rsimha
Copy link
Contributor

rsimha commented Jun 27, 2017

From https://travis-ci.org/ampproject/amphtml/jobs/247691456, it looks like you're now trying to use minified js in the unit tests shard, which only does a gulp build. I think we'd need to ensure that the validator minified js is built first. Is there a minimal alternative validator build that takes less than the full 2m 30s?

@dreamofabear dreamofabear force-pushed the test-example-validation-hermetic branch from e19575f to 3cea7ee Compare June 28, 2017 19:55
@dreamofabear dreamofabear merged commit ef438b0 into ampproject:master Jun 28, 2017
@dreamofabear dreamofabear deleted the test-example-validation-hermetic branch June 28, 2017 20:47
@dreamofabear dreamofabear mentioned this pull request Jul 6, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dreamofabear @rsimha @googlebot