🏗 gulp visual-diff refactor and stability fixes
#19327
Conversation
| const isInTestMode = /AMP_CONFIG=\{(?:.+,)?"test":true\b/.test( | ||
| fs.readFileSync('dist/amp.js', 'utf8')); | ||
| if (!isInTestMode) { | ||
| log('fatal', 'AMP was not build in test mode. Run', |
There was a problem hiding this comment.
nit: "The AMP runtime was not built in test mode."
… an exception instead of process.exit'ing
…all it explicitly, instead of as a pre-task in gulp
…re checking if it was killed
danielrozenberg
changed the title
--fortesting when running visual diff testsgulp visual-diff refactor and stability fixes
danielrozenberg
dismissed
rsimha’s stale review
This PR now includes multiple changes that weren't part of the original PR. Please take another look!
|
@danielrozenberg Seems like we still have builds that are failing in a non-clean manner: https://travis-ci.org/ampproject/amphtml/jobs/456056607#L640 |
|
Seems like an unrelated flaky test. I'll mark it flaky and assign Jon to fix |
|
The real problem I was pointing to was the message below. If a visual test flakes, the Travis job should exit cleanly and immediately, shouldn't it? I suspect this has to do with the instances of |
|
Ah I see. I'll create a new issue for this |
* Ensure that AMP was build with `--fortesting` when running visual diff tests * Address text nit * Add visual diff file to whitelist of files allowed to use AMP_CONFIG directly * Rewrite the log helper function to use appropriate streams, and throw an exception instead of process.exit'ing * Rename preVisualDiffTasks to ensureOrBuildAmpRuntimeInTestMode_ and call it explicitly, instead of as a pre-task in gulp * Cleanup after an unhandled Promise rejection * Cleanup should validate that the webServerProcess_ object exists before checking if it was killed * Refactor task into smaller functions * Add a workaround to the MaxListenersExceededWarning thrown by Node
Fixes:
webServerProcess_is defined before checking whether the process was killed, in the cleanup stepMaxListenersExceededWarningthat gets thrown by Node due to Puppeteer adding too manyDisconnectlistenersMinor changes:
process.exitcleanup_on unhandled Promise rejections as wellNew stuff:
--fortestingwhen running visual diff tests