add module build visual diffs

[erwinmombay]

add a module build visual diff job using the gulp serve --new_server task which does a transform of the documents

[amp-owners-bot]

Hey @rsimha! These files were changed:

.circleci/config.yml
build-system/pr-check/visual-diff-tests.js

Hey @danielrozenberg! These files were changed:

build-system/tasks/visual-diff/index.js

[rsimha]

I think a more maintainable way to do this is to make the job script visual-diff-tests.js take a command line arg, and parametrize the job definition in config.yml.

[danielrozenberg]

I think a more maintainable way to do this is to make the job script visual-diff-tests.js take a command line arg, and parametrize the job definition in config.yml.

#32808 has an example of how to do it

[rsimha]

PR is looking great! Just a few comments below.

[danielrozenberg]

Since both jobs use the same Percy write-only token, they'll both end up in the same project, and Percy will only report one as a status to GitHub - whichever of the two ends later would override the earlier.

Two possible solutions to this are:

1. create separate projects (ampproject/amphtml-module and ampproject/amphtml-nomodule) with their own tokens, passed as parameters in the job matrix
2. merge the two by running them in a single job by running the same test suite twice, on two executions of the server (with/without esm) and with adding (module)/(nomodule) to the individual snapshot names to differentiate between the two

I am very slightly leaning towards option #1, but don't really care either way. @rsimha can you think of how to pass the obfuscated token to the PR jobs?

[rsimha]

Since both jobs use the same Percy write-only token, they'll both end up in the same project, and Percy will only report one as a status to GitHub - whichever of the two ends later would override the earlier.

Oooh, excellent point, and good catch! TBH, after looking at the build logs on CircleCI and on Percy, I am leaning hard towards option 2. Here is my rationale:

• When you look at a Percy build for a single commit, it's useful to see all results (from module and nomodule) on one page. This way, if module breaks something but nomodule does not, it will be very obvious.
• Finalizing a visual build for a given commit from one CI job once and for all seems far simpler than trying to coordinate across two jobs, not to mention the cost of maintaining two tokens and two projects.
• Looking at the actual cost of parallelizing on CircleCI, VM setup / install takes ~2m, and running of all visual tests takes ~2m. I think it's better to test both build types from one job that runs for ~6m. (I assume it will work if we make the job wait for both the module and nomodule builds, download both, and run the correct kind of test server for each testing run.)

WDYT @erwinmombay @danielrozenberg? Maybe a little extra trouble up front to get this to work, but worth it in the end?

[danielrozenberg]

Again, no strong feelings either way. The overhead having two tokens/projects isn't that serious though, and we already discussed having separate projects for Chrome and for Firefox once Puppeteer fully supports the latter

[rsimha]

Again, no strong feelings either way. The overhead having two tokens/projects isn't that serious though, and we already discussed having separate projects for Chrome and for Firefox once Puppeteer fully supports the latter

Okay, in that case, I'll lower my hard preference to a mild preference, and let @erwinmombay chime in since he's going to be the primary user of the Percy builds. 😃

Re: adding new tokens to our CircleCI project, it's no trouble, and I can help do it. 👍

[erwinmombay]

@rsimha i think i would prefer option 2, just because seeing the results in a single page seems ideal

[rsimha]

Getting closer! A few more comments below, and two critical changes. (Right now, the PR is neither downloading ESM binaries nor serving them during testing 😃)

[rsimha]

A few more comments that I think will simplify the code.

[rsimha]

I see that in #30681, you added a call to buildNewServer() to integration.js. I think that change should be reverted because the server startup logic should entirely live in server.js. Other tasks should trust the server to start itself correctly. Can we clean that up too?

[erwinmombay]

agree with the statement, but buildNewServer isnt really start up logic and more compile logic. the code essentially isnt there (since its in typescript) before we make this call. buildNewServer i believe should be done before the task, i just placed it there since its the latest we can call it to make "lazy"

[erwinmombay]

do you think it would be imperative i make this change here, or can i follow up on a separate PR? I am working on it already but i wasn't sure if i could clean separate the issue out in another PR

[rsimha]

#32891 makes this change and cleans up integration.js. Once reviewed and merged, we shouldn't need any of these changes.

[danielrozenberg]

So... this still generates two separate builds and has Percy report the 2nd one

Basically you have to loop through both module and nomodule gulp serve inside the visual-diff/index.js job - everything needs to run from when launchPercyAgent() is called and finished before exitPercyAgent_() - this is what starts and finalizes a build on Percy

[erwinmombay]

got it. ill make the change

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.