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

Accept coverage info, collected from the browser #339

Open
canonic-epicure opened this issue Dec 22, 2021 · 11 comments
Open

Accept coverage info, collected from the browser #339

canonic-epicure opened this issue Dec 22, 2021 · 11 comments

Comments

@canonic-epicure
Copy link

I'm developing a cross-platform testing tool, which can run tests in browsers, Node and Deno: https://siesta.works

I'm trying to promote c8 as a recommended code coverage tool for it. It seems to work well for the Node.js tests.

However, if I collect coverage objects from the Chromium web pages, c8 does not recognize them as such. This is because Chromium's coverage object seems to have different format from Node's.

Node's format:

{
    "result": [{
        "scriptId": "7",
        "url": "file:///home/nickolay/workspace/siesta-workspace/siesta-monorepo/packages/siesta/bin/siesta.js",
        ...
    }],
    "source-map-cache": {
        "file:///home/nickolay/workspace/siesta-workspace/siesta-monorepo/node_modules/.pnpm/@web+dev-server@0.1.24_rollup@2.58.0/node_modules/@web/dev-server/dist/index.js": {
            "lineLengths": [13, 62, 137, 51, 133, 54, 127, 61, 148, 57, 130, 65, 142, 33],
        },

Chromium's format:

[
    {
        "scriptId": "32",
        "url": "http://localhost:8000/browser.js",
        ...
        "source": "..."
    }
]

So browser's format includes "raw" sources and does not contain the results property. Data from browser is collected with Playwright: https://playwright.dev/docs/api/class-coverage

I'm willing to contribute a PR normalizing this format difference when generating a report, would you accept it and may be provide some guidance?

@canonic-epicure
Copy link
Author

Hm.. It seems there's a fundamental assumption, that coverage info comes from the terminal process (http://domain/path urls are not supported at all, the node's path module is used to handle all urls). In browser case all urls are 'http'. I guess I'll have to convert the http urls to file://.

@canonic-epicure
Copy link
Author

So, by changing all urls to file:// with el.url = el.url.replace(/^https?:\/\//, 'file:///'), wrapping the results from browser with { result : ... }, using --allowExternal and applying this small patch to lib/report.js: canonic-epicure@0ed7994

I was able to receive decently looking report:
image

Sources are not shown of course:
image

This makes me positive its overall doable.

@bcoe
Copy link
Owner

bcoe commented Dec 25, 2021

@canonic-epicure this seems like a great feature request.

I think the logic might end up complicated enough for resolving URLs, and making it fit into the assumptions made by c8, that we should probably add a new helper, something like ./lib/util/chrome-to-v8.js, which would spit out a file that then works with v8-to-istanbul?

@canonic-epicure
Copy link
Author

canonic-epicure commented Dec 27, 2021

@bcoe

I think the logic might end up complicated enough for resolving URLs

Yes, would need to generalize the path module so that it could work with http urls.

we should probably add a new helper, something like ./lib/util/chrome-to-v8.js, which would spit out a file that then works with v8-to-istanbul

Probably. I'm open for suggestions and ideas. The thing is, it seems istanbul reports also assumes that all urls are file-system. So one would need to tweak them as well.

In theory, if the "generalized path" library would be created, it would be a matter of replacing the path symbol (plus may be inserting an async point for sources fetching, right now the sourceFinder in context is a synchronous method).

Right now I was able to workaround w/o patching c8, by changing all urls to file://. So I'm replacing http://localhost:8000/path/file.js to file://localhost/8000/path/file.js. Then I'm overriding the _getSourceMap method:

                    c8Report._getSourceMap = function (v8ScriptCov) {
                        return { source : v8ScriptCov.source }
                    }

and also run method (to add custom sourceFinder):

                    c8Report.run = async function () {
                        const context = istanbulLibReport.createContext({
                            // custom source finder
                            sourceFinder    : (url : string) => launcher.getSourcesOfCoverageFile(url),
                            dir             : this.reportsDirectory,
                            ...
                    }

Also need to enable allowExternal config, otherwise TestExclude tries to perform file-system resolution. And it all seems to work, just feels a bit hackish (and can show strange urls if port is included).

If you think its a sane feature request, I'm ready to contribute all the necessary code (after we come up with a plan).

@bcoe
Copy link
Owner

bcoe commented Dec 31, 2021

The thing is, it seems istanbul reports also assumes that all urls are file-system. So one would need to tweak them as well.

Updating Istanbul reports to not assume file system paths might be a good starting point. I think the reports most likely to make this assumption would be the HTML reports, which actually load files off disk I believe 🤔

If you think its a sane feature request, I'm ready to contribute all the necessary code (after we come up with a plan).

I think this sounds like a reasonable requests, users often ask how to handle browser coverage with c8, and there hasn't ever been a good answer.

With regards to coming up with a plan, perhaps a good starting point would be to draft a markdown RFC with a rough plan.

@canonic-epicure
Copy link
Author

With regards to coming up with a plan, perhaps a good starting point would be to draft a marddown RFC with a rough plan.

Ok, I'll be preparing one gradually. Will let you know.

@prantlf
Copy link

prantlf commented Dec 16, 2022

I was able to use the raw script coverage generated by puppeteer together with c8 report. My tests run in multiple pages served at http://localhost:5000:

// enable collecting JavaScript code coverage with raw coverage
await page.coverage.startJSCoverage({
  resetOnNavigation: false,
  includeRawScriptCoverage: true
})

// load one or more pages and run tests in them
...

// collect JavaScript code coverage
const coverage = await page.coverage.stopJSCoverage()

// extract raw script coverage with URLs replaced with absolute paths
const urlBase = 'http://localhost:5000/'
const cwd = process.cwd()
const result = coverage.map(({ rawScriptCoverage }) => {
  rawScriptCoverage.url = rawScriptCoverage.url.replace(urlBase, cwd)
  return rawScriptCoverage
})

// write the coverage like c8 expects it
await mkdir('coverage/tmp', { recursive: true })
await writeFile(`coverage/tmp/out.json`, JSON.stringify({ result }))

If you want to evaluate the coverage with c8 and not with nyc, there's no need to convert it with puppeteer-to-istanbul or v8-to-istanbul.

@Cactusbone
Copy link

Cactusbone commented Mar 31, 2023

Hello ! I'm trying to get c8 to report coverage from Nightwatch.js

From what I understand in this issue, what's missing is the sourcemap from url to local files.
From the first post, I believe the source-map-cache may be missing, however I do have a result property is the data from Chrome browser (using devtools-protocol).

First of all, should I create a new ticket for that ?

What would be the best way to get that sourcemap ?

I'm thinking of two ways, either I listen to Debugger.scriptParsed events from devtools-protocol, and use the sourceMapURL to match the scriptId (looks like I'm out of luck SeleniumHQ/selenium#10804). Or I can download the bundles from url in coverage output and then source maps from comments.

Does this looks like a good way to get usage coverage ? Am I missing something ? any tips ?

Thanks!

@prantlf
Copy link

prantlf commented Apr 1, 2023

@Cactusbone, aren't the source maps in the file system together with the bundled assets? You shouldn't need to fetch them using the browser devtools protocol. I use the code above to catch code coverage in the source files, with the help of the source maps:

Sources:

src
├── bundled-engine.js
├── graph.js
├── index.js
├── renderer.js
├── script-editor.js
└── separate-engine.js

Build output:

dist
├── index.min.js      (ends with "//# sourceMappingURL=index.min.js.map")
└── index.min.js.map  (contains source paths like ../src/graph.js)

Test run:

❯ node test  (writes coverage/tmp/out.json)

Configuration in package.json:

"c8": {
  "excludeAfterRemap": true,
  "exclude": [
    "src/codejar",
    "src/prism"
  ],
  "reporter": [
    "lcov",
    "text"
  ],
  "checkCoverage": true,
  "branches": 100,
  "functions": 100,
  "lines": 100,
  "statements": 100
}))

Coverage check:

❯ npx c8 report
--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------------|---------|----------|---------|---------|-------------------
All files           |    98.4 |      100 |     100 |    98.4 |
 bundled-engine.js  |     100 |      100 |     100 |     100 |
 graph.js           |     100 |      100 |     100 |     100 |
 renderer.js        |     100 |      100 |     100 |     100 |
 script-editor.js   |   96.84 |      100 |     100 |   96.84 | 71-76
 separate-engine.js |     100 |      100 |     100 |     100 |
--------------------|---------|----------|---------|---------|-------------------
ERROR: Coverage for lines (98.4%) does not meet global threshold (100%)
ERROR: Coverage for statements (98.4%) does not meet global threshold (100%)

(excludeAfterRemap doesn't work well, but it's another story...)

@Cactusbone
Copy link

@prantlf Thanks a lot ! This is what I was missing :)

Since we're not storing either the bundles nor the sources on the filesystem, It wasn't working for me (and the raw script coverage mentions in this ticket misled me). I'll see what I can do (either store them on the filesystem, or inline them using "source-map-cache").

@Cactusbone
Copy link

Cactusbone commented Apr 5, 2023

Yes, it works !

I had to save both the bundles and its map at the root of the projet, and add the bundles to my include section too.

However, with or without excludeAfterRemap, I get 100% coverage of way too many files (just like the issue you opened about excludeAfterRemap). I'll look into it :)

and from the code, it's not possible, for example with a single passage in a function containing a if/else statement, I get both the if and the else paths covered

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants