WIP: Enabling test coverage on e2e tests #31787

rcebulko · 2021-01-04T18:52:09Z

Current state of affairs:

gulp (build|dist) --coverage [--fortesting, --extensions, etc.] creates binaries with instrumented code
gulp serve --coverage=live runs the server with automatic code coverage collection
- This works by appending an event handler to v0.js that reports to the coverage endpoint on page unload
- This code lives in amphtml/build-system/server/app.js
gulp e2e --coverage runs the e2e test suite with code coverage collection
- This works by extracting istanbul's coverage variable (__coverage__) from the page context and aggregating it after each test case (see amphtml/build-system/tasks/e2e/describes-e2e.js)
- After all tests are complete, the aggregated coverage object is reported to the endpoint (see amphtml/build-system/tasks/e2e/index.js)
- The coverage report is opened automatically upon test suite completion
Currently, a handful of tests fail for me
- Some are timeout or other similar errors that don't seem related, but I'm not yet certain
- amp-ad-exit tests almost all fail with an error message like javascript error: cov_1xqb08d7d9 is not defined
- istanbul instrumentation is configured to use the name __coverage__ for the coverage variable, suggesting these may be references to instrumentation Karma is trying to automatically provide somehow
amphtml/build-system/tasks/codecov-upload.js is already configured for e2e test coverage
- If the e2e coverage data is created when gulp codecov-upload is run, it will be reported with the e2e_tests tag
- This is the same way unit and integration test coverage is reported
The file e2e.logcontained first the output from a no-coverage one, followed by the output from a coverage run
- First run command: gulp build --fortesting && gulp e2e --nobuild --headless
- Second run command: gulp build --fortesting --coverageh && gulp e2e --nobuild --headless --coverage
- Diff: b11e565

rcebulko · 2021-01-04T19:13:53Z

rcebulko · 2021-01-04T19:14:26Z

.travis.yml

@@ -51,7 +51,7 @@ jobs:
    - stage: build
      name: 'Unminified Build'
      script:
-        - unbuffer node build-system/pr-check/unminified-build.js
+        - travis wait 30 unbuffer node build-system/pr-check/unminified-build.js


Without this, the longer --coverage build will timeout on Travis

As written, this will increase the time taken by all Travis builds because the "test" stage will now have to wait for (up to) a 30 minute "build" stage.

The good news is that we should be able to solve this problem with the move to CircleCI, which I believe allows for more sophisticated matrices (e.g. create a separate independent "coverage" stage, which starts at the beginning and runs in parallel with the "build" and "test" stages, which run serially.)

I expect to make progress on this in the next week or so, will loop you in when the coast is clear to try this out.

Thanks for reminding me about this, forgot to note it! My thinking was this would only be viable for something like the Nightly builds, and probably shouldn't be a part of CI during the development day. Even if Circle CI supports splitting the work off, e2e test coverage shouldn't fluctuate massively from one commit to another, and daily data would be sufficient as a metric.

rcebulko · 2021-01-04T19:14:43Z

build-system/pr-check/e2e-tests.js

@@ -67,7 +67,7 @@ async function main() {
    ) {
      downloadDistOutput(FILENAME);
      timedExecOrDie('gulp update-packages');
-      timedExecOrDie('gulp e2e --nobuild --headless --compiled');
+      timedExecOrDie('gulp e2e --nobuild --headless --compiled --coverage');


This should eventually be gated with argv.coverage; likewise with the gulp dist/gulp build commands below, which I had just enabled --coverage on while experimenting/trying to get it to run on Travis.

Agree. Adding an additional job to the CI matrix isn't that expensive, so we should probably just do all coverage measurements in a parallel job.

rcebulko · 2021-01-04T19:17:46Z

src/amp.js

@@ -64,7 +64,9 @@ import {stubElementsForDoc} from './service/custom-element-registry';
 * @type {boolean|undefined}
 */
 const shouldMainBootstrapRun = !self.IS_AMP_ALT;
-
+// NO SUBMIT PLZ


This change was to trigger build targets, "DO NOT SUBMIT" will make Travis fail before running the tests themselves

rsimha

Thanks for the work so far, @rcebulko. I've added a couple of comments below.

rsimha · 2021-01-06T17:40:05Z

.travis.yml

@@ -51,7 +51,7 @@ jobs:
    - stage: build
      name: 'Unminified Build'
      script:
-        - unbuffer node build-system/pr-check/unminified-build.js
+        - travis wait 30 unbuffer node build-system/pr-check/unminified-build.js


As written, this will increase the time taken by all Travis builds because the "test" stage will now have to wait for (up to) a 30 minute "build" stage.

The good news is that we should be able to solve this problem with the move to CircleCI, which I believe allows for more sophisticated matrices (e.g. create a separate independent "coverage" stage, which starts at the beginning and runs in parallel with the "build" and "test" stages, which run serially.)

I expect to make progress on this in the next week or so, will loop you in when the coast is clear to try this out.

rsimha · 2021-01-06T17:41:51Z

build-system/pr-check/e2e-tests.js

@@ -67,7 +67,7 @@ async function main() {
    ) {
      downloadDistOutput(FILENAME);
      timedExecOrDie('gulp update-packages');
-      timedExecOrDie('gulp e2e --nobuild --headless --compiled');
+      timedExecOrDie('gulp e2e --nobuild --headless --compiled --coverage');


Agree. Adding an additional job to the CI matrix isn't that expensive, so we should probably just do all coverage measurements in a parallel job.

rsimha · 2021-01-06T17:52:08Z

e2e.log

+          AMPHTML ads FIE environment 
+
+             product1 ad opened:
+     JavascriptError: javascript error: cov_1xqb08d7d9 is not defined


Searching for this error led me to istanbuljs/istanbuljs#499, which mentions among other things the fact that a babel transform might be hoisting the coverage variable.

I wonder if it's possible to / worth playing with the order of babel plugins in test-config.js, pre-closure-config.js, and unminified-config.js by moving the instrumentation transform to the end.

Side note: I misspelled the plugin name as instanbulPlugin in test-config.js :)

That sounds like a very plausible culprit. It's certainly possible to play with the order of plugins, and I did a lot of experimentation with varied results (though I do not recall every variation I attempted). TL;DR - Sometimes re-ordering results in coverage being generated for the binaries, rather than the source files. In theory it should be possible to do instrumentation at any step and transform the source maps along the way, but I never managed to get that working.

rcebulko added 8 commits December 7, 2020 11:34

Change amp.js to force tests

f8ef412

build with coverage instrumentation

857939e

travis_wait

f4071e4

Run e2e with --coverage

41c7efb

Move coverage reporting to end of e2e testing

33cab53

Add --coverage to gulp dist in Travis dist

67ef853

Add log from e2e tests without coverage

372e38a

Add log from e2e tests with coverage

b11e565

rcebulko commented Jan 4, 2021

View reviewed changes

rsimha reviewed Jan 6, 2021

View reviewed changes

rcebulko closed this Apr 12, 2021

rcebulko deleted the e2ecov branch April 12, 2021 15:39

rsimha mentioned this pull request Apr 22, 2021

Accurately measure code coverage during integration tests #22682

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Enabling test coverage on e2e tests #31787

WIP: Enabling test coverage on e2e tests #31787

rcebulko commented Jan 4, 2021 •

edited

rcebulko commented Jan 4, 2021

rcebulko Jan 4, 2021

rsimha Jan 6, 2021 •

edited

rcebulko Jan 6, 2021

rcebulko Jan 4, 2021

rsimha Jan 6, 2021

rcebulko Jan 4, 2021

rsimha left a comment

rsimha Jan 6, 2021 •

edited

rsimha Jan 6, 2021

rsimha Jan 6, 2021 •

edited

rcebulko Jan 6, 2021

WIP: Enabling test coverage on e2e tests #31787

WIP: Enabling test coverage on e2e tests #31787

Conversation

rcebulko commented Jan 4, 2021 • edited

rcebulko commented Jan 4, 2021

rcebulko Jan 4, 2021

Choose a reason for hiding this comment

rsimha Jan 6, 2021 • edited

Choose a reason for hiding this comment

rcebulko Jan 6, 2021

Choose a reason for hiding this comment

rcebulko Jan 4, 2021

Choose a reason for hiding this comment

rsimha Jan 6, 2021

Choose a reason for hiding this comment

rcebulko Jan 4, 2021

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

rsimha Jan 6, 2021 • edited

Choose a reason for hiding this comment

rsimha Jan 6, 2021

Choose a reason for hiding this comment

rsimha Jan 6, 2021 • edited

Choose a reason for hiding this comment

rcebulko Jan 6, 2021

Choose a reason for hiding this comment

rcebulko commented Jan 4, 2021 •

edited

rsimha Jan 6, 2021 •

edited

rsimha Jan 6, 2021 •

edited

rsimha Jan 6, 2021 •

edited