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

feat(@angular-devkit/build-angular): code coverage for ng build, ng serve and ng e2e #13131

Closed
wants to merge 1 commit into from

Conversation

divdavem
Copy link
Contributor

@divdavem divdavem commented Dec 4, 2018

This PR is related to #6286 and allows --code-coverage and --code-coverage-exclude options to be used with ng build, ng serve and ng e2e, so that it is easy to instrument the code for coverage.

When used with these commands, the --code-coverage switch does not automatically create a file with the report but allows the developers to access the __coverage__ variable from Istanbul.js inside the browser to get coverage data.
That object can then be used to generate a report as described in https://istanbul.js.org/docs/advanced/coverage-object-report/

@ngbot
Copy link

ngbot bot commented Dec 6, 2018

Hi @divdavem! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

…d, ng serve and ng e2e

This commit allows --code-coverage and --code-coverage-exclude options
to be used with ng build, ng serve and ng e2e, so that it is easy to
instrument the code for coverage.

When used with these commands, the --code-coverage switch does not
automatically create a file with the report but allows the developers to
access the __coverage__ variable from Istanbul.js inside the browser to
get coverage data.
That object can then be used to generate a report as described in
https://istanbul.js.org/docs/advanced/coverage-object-report/
Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

@divdavem this PR looks incomplete.

I understand the part where it allows instrumenting the code in builds in general, but it doesn't actually do anything with that instrumentation (e.g. using that PR and running ng e2e --code-coverage does not give me any coverage over end-to-end tests).

It is unclear to me how it is meant to be used. There are also no tests, which are required for new features.

Before investing more time in this feature perhaps it would be better to discuss the general with which you want to tackle this feature, so we can come to an agreement before developing it.

@divdavem
Copy link
Contributor Author

@filipesilva Thank you for your answer. I am glad that a human (and not only bots) looked at my PR!

It is unclear to me how it is meant to be used.

I explained in this comment of #6286 how this new feature can be used with e2e tests:

For info, I have opened PR #13131 to simplify the instrumentation of files, so that it is possible to use --code-coverage and --code-coverage-exclude with ng build ng serve or ng e2e.
This only instruments files as part of the build done by ng.

To get a coverage report, it is then still necessary to record the coverage, for example with an onComplete function in the protractor configuration:

async onComplete() {
  console.log('Retrieving coverage...');
  const coverage = await browser.executeScript('return JSON.stringify(window.__coverage__);');
  const coverageFile = path.join(__dirname, ".nyc_output", "out.json");
  console.log(`Coverage retrieved (${coverage.length} bytes), saving it to ${coverageFile}`);
  fs.writeFileSync(coverageFile, coverage);
  console.log("Coverage saved successfully!");
};

And then it is possible to produce both the lcov and html reports with

nyc report --reporter=lcov --report-dir=coverage-e2e

The simple change that I did in this PR simplifies a lot the current steps (detailed in https://github.com/edvlucas/angular-e2e-coverage) that have to be done to instrument the code to get a coverage report for e2e tests.

More generally, I think that being able to produce instrumented code that can be used just like non-instrumented code but also fills a __coverage__ variable with coverage information when it is executed is interesting.

There are also no tests, which are required for new features.

I will add some tests.

using that PR and running ng e2e --code-coverage does not give me any coverage over end-to-end tests

If you think it is required to include, as part of this PR, the production of the coverage report for e2e tests (instead of only the instrumentation), we indeed, as you said, have to discuss how to best implement the collection of coverage results.
A basic idea would be to wrap the configuration of protractor to add something similar to the above onComplete function. How should we do that? We could call protractor with a different file than the one configured, and that file would call the original configuration file, and return a modified configuration... but then we would need to also update the paths that are interpreted as relative to the configuration file, and this does not seem very clean.
Also, in order to support all use cases, it is probably not sufficient to only collect coverage at the end from the current window, because e2e tests can possibly cause multiple pages to be opened or navigated to in the browser. In order to get the complete coverage, it is needed to collect the coverage from all browser windows that run instrumented code.
To solve this issue, another idea would be to let application developers call a provided function (with an optional reference to a browser window as an argument) from their protractor configuration or from their tests to collect coverage from that browser window (or the default browser window) at that time...

Please tell me what you think.

@filipesilva filipesilva added the needs: discussion On the agenda for team meeting to determine next steps label Dec 11, 2018
@filipesilva
Copy link
Contributor

Thanks for explaining the usage. My worry with this usage pattern is that adding the --code-coverage option in the current way does not add a feature as much as it exposes an API that then needs to be used.

This, I feel, is not ideal. A users expectation for ng e2e --code-coverage would be similar to ng test --code-coverage: for a code coverage report to be generated. Instead this option is more of a ng e2e --expose-code-coverage-object (as per https://istanbul.js.org/docs/advanced/coverage-object-report/).

And at this point it's mostly a custom webpack config change, akin to what https://github.com/manfredsteyer/ngx-build-plus is usually used for.

It's not trivial to create a robust user journey for this feature though. Like you mentioned, it would require wrapping the users protractor config, and to allow for multiple testing pattern (multiple page open/navigation/etc).

Let me bring this up with team to see what they think.

@divdavem
Copy link
Contributor Author

Thank you for your answer.

We could also add something like the following onComplete function as part of the default protractor.conf.js file:

  async onComplete() {
    const coverageData = await browser.executeScript('return JSON.stringify(window.__coverage__);');
    if (coverageData) {
      const { createReporter: createCoverageReporter } = require('istanbul-api');
      const { createCoverageMap } = require('istanbul-lib-coverage');
      const coverageMap = createCoverageMap(JSON.parse(coverageData));
      const reporter = createCoverageReporter();
      reporter.dir = require('path').join(__dirname, 'coverage-e2e');
      reporter.addAll(['json', 'lcov', 'text'])
      reporter.write(coverageMap);
    }
  }

This would automatically generate the report in the coverage-e2e folder for simple cases if --code-coverage is used, and still let users change the logic if needed (if they reload a page or open multiple pages in their tests).

@mgechev
Copy link
Member

mgechev commented Dec 13, 2018

Thank you for the contribution! We discussed this feature today. Although we think it's out of the scope of the CLI at this point, we believe it could be beneficial for our users. I'd recommend you to look at @manfredsteyer's ngx-build-plus and consider letting folks take advantage of e2e test coverage through a plugin.

@mgechev mgechev closed this Dec 13, 2018
@hansl hansl removed needs: discussion On the agenda for team meeting to determine next steps labels Jan 24, 2019
@beckmx
Copy link

beckmx commented Mar 15, 2019

@divdavem I need this feature, my company is using cypress and I need a way to extract the coverage, is your feature implemented in a package or something?

@divdavem
Copy link
Contributor Author

@divdavem I need this feature, my company is using cypress and I need a way to extract the coverage, is your feature implemented in a package or something?

@beckmx I did not implement the feature in a package. You can have a look at this PR I did on ng-bootstrap for an example on how to implement it on an application.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants