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

Jasmine builtin coverage #874

Closed

Conversation

Toxicable
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@Toxicable
Copy link
Author

First attempt at adding coverage to nodejs_binary as a builting feature, rather than just jasmine_node_test.
In it's current state this will not work.

Using NODE_V8_COVERAGE nodejs(or v8?) will only write the coverage report once the process exits. Since the jasmine_runner.js is effectively apart of the users code, it cannot access the coverage report.

One solution here would be to add another program to node_launcher.sh that only runs when coverage is enable that collects up the coverage report and turns it into a human readable report for the dev or stores it away for bazel to use.

@Toxicable Toxicable changed the title Jasmine builting coverage Jasmine builtin coverage Jun 19, 2019
@Toxicable
Copy link
Author

Put in a feature request here: nodejs/node#28283
Which would solve the above issues.

@Toxicable
Copy link
Author

Ok so the solution here is still to use the inspector api, with a few extra flags.
So no need to add the coverage flag to nodejs_binary
However, this is now blocked on #823
To resolve better source maps

@Toxicable Toxicable mentioned this pull request Sep 18, 2019
4 tasks
@Toxicable
Copy link
Author

replaced with #1135

@Toxicable Toxicable closed this Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants