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.

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

🏗 Introduction of coverage-map task. #29519

Merged
merged 35 commits into from
Aug 3, 2020

Conversation

xiexr151e
Copy link
Contributor

This PR attempts to incorporate puppeteer and source map explorer into one task, and thereby automating the generation of code coverage heat maps in v0.js.

package.json Outdated Show resolved Hide resolved
@rsimha rsimha requested a review from rcebulko July 28, 2020 16:37
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.

I'd be interested in seeing if this approach works, and if it can generate coverage numbers for minified v0.js (and potentially, unminified amp.js) during tests that use HTML files that in turn load the runtime and extensions from a server.

Adding @rcebulko as FYI / additional reviewer since this is relevant to coverage.

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Could this be tweaked to allow generating coverage maps for extension files as well? The list of files to include might be a useful thing to set as a flag

@xiexr151e
Copy link
Contributor Author

I'd be interested in seeing if this approach works, and if it can generate coverage numbers for minified v0.js (and potentially, unminified amp.js) during tests that use HTML files that in turn load the runtime and extensions from a server.

Adding @rcebulko as FYI / additional reviewer since this is relevant to coverage.

Unfortunately, for whatever reason, unminified JS does not work with source map explorer - I've manually tested this.

@xiexr151e
Copy link
Contributor Author

Could this be tweaked to allow generating coverage maps for extension files as well? The list of files to include might be a useful thing to set as a flag

Maybe. Source map explorer requires the JS file, the corresponding source map, and a coverage JSON file (which we get through puppeteer tests).
The documentation for source map explorer can be found here: https://www.npmjs.com/package/source-map-explorer

build-system/tasks/coverage-map/coverage-map.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/coverage-map.js Outdated Show resolved Hide resolved
@dvoytenko
Copy link
Contributor

@xiexr151e looks great. just revert the root package.json and I'll approve this pull request.

@xiexr151e
Copy link
Contributor Author

Screen Shot 2020-07-31 at 11 16 28 AM

For some reason, Travis fails, but I'm not sure why this is happening.
@rsimha

async function coverageMap() {
installPackages(__dirname);

puppeteer = require('puppeteer');
Copy link
Contributor

Choose a reason for hiding this comment

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

@erwinmombay instead of doing this, can we instead do a dynamic require in the main gulpfile.js when we load this module?

Copy link
Member

Choose a reason for hiding this comment

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

sorry i missed this, do you mean as a global of some sort? I'll defer to @rsimha since we are following their pattern

@rsimha
Copy link
Contributor

rsimha commented Jul 31, 2020

For some reason, Travis fails, but I'm not sure why this is happening.

These are due to the changes in the root yarn.lock. Since you've moved all packages to the task directory, you can simply revert the root package.json and yarn.lock.

Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

The shape of this pull request is now correct. I'm approving it.

build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Show resolved Hide resolved
@erwinmombay erwinmombay merged commit a1ef31d into ampproject:master Aug 3, 2020
@xiexr151e xiexr151e deleted the derek-coverage-task branch August 4, 2020 04:34
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* moved script to tasks and added package requirements

* lint fix

* progress on making task

* remove test.json

* additional lint fixes

* fixed according to comments

* several changes in coverage-map

* lint

* clarify description

* change to use async writefile

* review flag

* OK in window.scrollBy()

* lint

* added source-map-explorer to package.json

* Some changes based on feedback

* additional updated based on feedback

* small change

* added separate package.json

* package specific changes

* undo changes in root package.json

* added yarn.lock

* several changes

* revert yarn.lock

* lock file

* delete package-lock.json

* update gulpfile.js

* lazy importing

* revert change

* remove package-lock

* Update package.json

* yarn.lock

* prettify

* excluded coverage-map for getComputedStyle

* additional feedback

Co-authored-by: Derek Tse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants