Improve docs - typescript/sourcemaps

HarelM commented 8 months ago

I have read what I believe is the relevant part of the docs that talks about puppeteer and also tried to implement it, but I get a report that doesn't take into consideration the source mapping. I'm not sure what I missed in the configuration and I couldn't find anything in the readme that talks about it. Here's how my report looks:

The code I'm using is a copy-paste of the example provided in the docs basically:

const coverage = await page.coverage.stopJSCoverage();
    await page.close();
    if (!reportCoverage) {
        return;
    }
    // to raw V8 script coverage
    const coverageList = [... coverage.map((it) => {
        return {
            source: it.text,
            ... it.rawScriptCoverage
        };
    })];

    const coverageOptions: any = {
        // logging: 'debug',
        // watermarks: [60, 90],
        reports: [
            ['console-summary', {
                metrics: ['bytes', 'functions', 'lines']
            }],
            ['v8', {
                metrics: ['bytes', 'functions', 'lines']
            }]
        ],

        name: 'My puppeteer Coverage Report',
        assetsPath: '../assets',
        lcov: true,

        sourceFilter: (sourcePath) => sourcePath.search(/src\//) !== -1 || sourcePath.search(/minify\//) !== -1,
        outputDir: './coverage/puppeteer'
    };
    await MCR(coverageOptions).add(coverageList);
    const coverageResults = await MCR(coverageOptions).generate();
    console.log('puppeteer coverage reportPath', coverageResults.reportPath);

cenfun commented 8 months ago

Did you enable sourcemap when building for the dist file? see here Collecting V8 Coverage Data

enable sourcemap and do not compress/minify:
- webpack: devtool: source-map and mode: development, example webpack.config-v8.js
- rollup: sourcemap: true
- vite: sourcemap: true and minify: false
- esbuild: sourcemap: true and minify: false

There should be a separate sourcemap file will be created:

dist/maplibre-gl-dev.js
dist/maplibre-gl-dev.js.map  <- sourcemap file

When you start puppeteer, the page should loading the html with the scripts like:

<html>
<body><script src="dist/maplibre-gl-dev.js"></script><body>
</html>

// enable coverage
await page.coverage.startJSCoverage({includeRawScriptCoverage: true});

// open above html
await page.goto("http://localhost:8080/index.html");

// run your tests ...

// collect coverage data
const coverage = await page.coverage.stopJSCoverage();

HarelM commented 8 months ago

sourcemaps are enabled, You can inspect the package content here (last pre-release version, which is very similar to main branch): https://unpkg.com/browse/maplibre-gl@4.0.0-pre.4/dist/ I run my tests here: https://github.com/maplibre/maplibre-gl-js/blob/7decb26cd6895ad595c19bd2cb6777769996c17f/test/integration/render/run_render_tests.ts These are the changed I made locally to try this out: Let me know what other information might help you to assist me.

Thanks for the quick response!!

cenfun commented 8 months ago

According to the link you provided, I found that you load the js via page.addScriptTag

await page.addScriptTag({path: 'dist/maplibre-gl-dev.js'});

Actually, there is a sourcemap link in the maplibre-gl-dev.js: The sourcemap will be failed to load without over the http or https. (cause the sourcemap path is wrong now)

Is it possible to start a local web server for the static files?

HarelM commented 8 months ago

I suppose it's possible, but isn't there an easier way to load the sourcemap? Maybe using fs API? the coverage part is executed in node basically, isn't it?

cenfun commented 8 months ago

Yes you can, following is what i did:

import { CoverageReport } from 'monocart-coverage-reports';

async function closePageAndFinish(page: Page, reportCoverage: boolean) {
    const coverage = await page.coverage.stopJSCoverage();
    await page.close();
    if (!reportCoverage) {
        return;
    }

    const rawV8CoverageData = coverage.map((it) => {
        // Convert to raw v8 coverage format
        return {
            source: it.text,
            ...it.rawScriptCoverage
        };
    });

    rawV8CoverageData.forEach((entry:any) => {
        // read sourcemap for the maplibre-gl-dev.js manually
        if (entry.url.endsWith('maplibre-gl-dev.js')) {
            entry.sourceMap = JSON.parse(fs.readFileSync('dist/maplibre-gl-dev.js.map').toString('utf-8'));
        }
    });

    const mcr = new CoverageReport({
        // logging: 'debug',
        name: 'My Coverage Report',
        outputDir: './coverage-reports'
    });
    mcr.cleanCache();

    await mcr.add(rawV8CoverageData);

    await mcr.generate();

    // const converter = v8toIstanbul('./dist/maplibre-gl-dev.js');
    // await converter.load();
    // converter.applyCoverage(coverage.map(c => c.rawScriptCoverage!.functions).flat());
    // const coverageReport = converter.toIstanbul();
    //const report = JSON.stringify(rawV8CoverageData);
    //fs.mkdirSync('./coverage', {recursive: true});
    //fs.writeFileSync('./coverage/coverage-render.json', report);
}

and report (I just removed most of test cases because it's slow, so the coverage is low)

HarelM commented 8 months ago

Great! Thanks! I've published a branch and I'll look into the coverage report when the run is finished. I think it's worth mentioning what you wrote here about the sourcemaps in the main readme or in the puppeteer code example.

HarelM commented 8 months ago

This is obviously a lot better, but there's still some issues, see here for example the last closing bracket: Also this variable declaration _prevEase:

See here the full report for this file: https://app.codecov.io/gh/maplibre/maplibre-gl-js/pull/3598/blob/src/ui/handler/scroll_zoom.ts

If I needed to guess, this is probably a source mapping issue, but I have no clue how to investigate this.

cenfun commented 8 months ago

Thanks for your suggestion. By the way, you can try the new report codecov which I just added yesterday.

reports: [
        'json',
        'html',
        'codecov'
    ],

there will be a codecov.json created in the output dir, and use it for Codecov


      - name: Codecov
        uses: codecov/codecov-action@v3
        with:
          files: ./your-output-dir/codecov.json

HarelM commented 8 months ago

The original report, without the render tests (e2e) can be found here: https://app.codecov.io/gh/maplibre/maplibre-gl-js/pull/3589/blob/src/ui/handler/scroll_zoom.ts This is how this method looks - which I think explains the difference:

HarelM commented 8 months ago

By the way, you can try the new report codecov which I just added yesterday.

Codecov can parse the json output, why should I use codecov and not json?

cenfun commented 8 months ago

Actually not, the json you used is istanbul json format, Codecov will convent it to adapter codecov UI, so it maybe not perfect. it's worth to try codecov report.

BTW, Please try v8 report too, which is perfect to show the native V8 coverage data.

HarelM commented 8 months ago

Ok, I've pushed a commit to the branch to switch from json to codecov. I've ran locally the render (e2e) tests and the following is what I get with v8 for the scroll_zoom.ts file:

So I think the problem is the merge between jest coverage (which doesn't report the last bracket as covered and the initialization of an object as covered) and this coverage which reports them as uncovered.

Any idea how to solve this?

cenfun commented 8 months ago

I don't think we can merge the coverage data with jest's. Which coverage provider do you used in jest? v8 or istanbul? Maybe it can be merged if jest can provide the raw v8 coverage data. So you used multiple test runners, which makes merging data complex.

HarelM commented 8 months ago

I'm running the following line in jest: npm run jest-ci -- --coverage --coverageReporters json text html-spa --selectProjects unit And upload the json file, so I guess istanbul...?

The report for codecov json is 83% coverage and for istanbul is 84% (in my PR), it seems like the difference is due to partial missing lines, the total lines is the same.

cenfun commented 8 months ago

Jest uses istanbul by default, I think it's better to revert to using json.

HarelM commented 8 months ago

Sure, no problem. Do you know of a way to force jest to report the closing bracket as covered?

HarelM commented 8 months ago

Or is there a way to tell this tool to avoid reporting the closing brackets as not covered? I guess the problem here is, as you mentioned, the different reporting "styles" of the different tools. Am I doing something that exotic? I can't find a lot of information about it when googling...

cenfun commented 8 months ago

I don't think it's possible. When Jest uses istanbul, It requires the source code is instrumented with Istanbul, see introduction, it seems the closing bracket never be counted. (it might be a problem when merging with v8 which counting closing bracket)

When the coverage data provided by V8, it counts the byte ranges, so it's prefect to show the covered/uncovered ranges

see v8 introduction https://v8.dev/blog/javascript-code-coverage

HarelM commented 8 months ago

So, what can I do to solve this? Is there a way for me to overcome this difference?

cenfun commented 8 months ago

You should use a unified test framework. Either use Jest and complete all tests include run_render_tests through Jest. or convert all Jest tests to run_render_tests

cenfun commented 8 months ago

I recommend using @playwright/test as a testing framework. see https://playwright.dev/docs/api/class-test It is excellent enough to complete e2e tests, and it should be the fastest testing framework at present.

HarelM commented 8 months ago

You should use a unified test framework.

That doesn't make any sense, sorry... Jest are used for unit testing and ran in node environment, where everything is mocked and there's no need to open a browser, while render tests (e2e) are using puppeteer, and use the browser. I don't think I can use the same framework for both. I can try and use jest for the render tests, but I'm not sure I'll get any coverage, as these are e2e tests which uses the browser. I also can't convert the render tests to node environment, as it will not test what I need to test (it was in node in the past and there was a need to mock so many things the tests weren't really reliable).

From my point of view, we are using the most popular framework for each type of test. There are a lot of unit test frameworks, but they are not as strong and as popular as jest, which has a huge community and a lot of resources to solve mocking problems, which is where other test framework falls short.

For example the following is the mock part of the docs of playwright: https://playwright.dev/docs/mock Very lacking: how do I mock a class? how do I substitute a call of a class method? these are basic unit test scenarios.

We are mocking canvas, webgl, DOM, touch events etc in the unit test, if these are not solved in other test framework I don't see a good reason to change. The render test on the other hand is a single simple class, I don't mind rewriting it in any framework, but I need to know I get the right coverage (same as jest), otherwise why bother?

cenfun commented 8 months ago

Yes you are right. It is better to use Jest as the unit test tool. There's another option you can try which is test render_tests with istanbul. 1, Instrumenting source code with Istanbul, see rollup-plugin-istanbul 2, Collecting coverage data from window.__coverage__

 const coverageData = await page.evaluate(() => window.__coverage__);

see https://github.com/cenfun/monocart-coverage-reports?tab=readme-ov-file#collecting-istanbul-coverage-data both are istanbul coverage data, they should be able to merge perfectly.

HarelM commented 8 months ago

Interesting, I'll need to understand how to properly instrument the code and make sure it works. THANKS for all the help and guidance!

HarelM commented 8 months ago

Seems like the plugin has an issue with assert {type 'json' } :-/ I've opened the following feature request: https://github.com/artberri/rollup-plugin-istanbul/issues/60 I honestly am starting to think I'm the crazy one, with code that is not similar to anyone else O_o

cenfun commented 8 months ago

Yes, Instrumenting source code for Istanbul could be quite painfull. Different build tools requires different plugins, and when you use non-native JS languages such as ts, vue, may not be supported well. That's why I prefer V8.

I am planning to create a new Jest reporter to generate V8 reports, see repo https://github.com/cenfun/jest-monocart-coverage It's still under development.

HarelM commented 8 months ago

Interesting, keep me posted, I'd be happy to take it to a test drive.

cenfun commented 8 months ago

The custom reporter of Jest has been released: https://github.com/cenfun/jest-monocart-coverage And, the new coverage reporter supports data merging. Let's use 'scroll_zoom.ts' as an example, this is the generated results:

The left side is test-render, the middle is jest-unit, and the right side is merged. It seems working well, but it might require more verification. The general workflow following:

Install `jest-monocart-coverage`

npm i jest-monocart-coverage -D

1, test-render, using `raw` report to export original coverage data to `./coverage-reports/render/raw`

// run_render_tests.ts
import {CoverageReport} from 'monocart-coverage-reports';
async function closePageAndFinish(page: Page, reportCoverage: boolean) {
    const coverage = await page.coverage.stopJSCoverage();
    await page.close();
    if (!reportCoverage) {
        return;
    }

    const rawV8CoverageData = coverage.map((it) => {
        // Convert to raw v8 coverage format
        return {
            source: it.text,
            ...it.rawScriptCoverage
        };
    });

    rawV8CoverageData.forEach((entry:any) => {
        // read sourcemap for the maplibre-gl-dev.js manually
        if (entry.url.endsWith('maplibre-gl-dev.js')) {
            entry.sourceMap = JSON.parse(fs.readFileSync('dist/maplibre-gl-dev.js.map').toString('utf-8'));
        }
    });

    const mcr = new CoverageReport({
        // logging: 'debug',
        name: 'My Render Coverage Report',
        reports: [
            ['v8'],
            ['raw'],
            ['console-summary']
        ],

        sourceFilter: (sourcePath) => {
            return !sourcePath.includes('node_modules/') && sourcePath.search(/src\//) !== -1;
        },

        outputDir: './coverage-reports/render'
    });
    mcr.cleanCache();

    await mcr.add(rawV8CoverageData);

    await mcr.generate();

    // const converter = v8toIstanbul('./dist/maplibre-gl-dev.js');
    // await converter.load();
    // converter.applyCoverage(coverage.map(c => c.rawScriptCoverage!.functions).flat());
    // const coverageReport = converter.toIstanbul();
    // const report = JSON.stringify(coverageReport);
    // fs.mkdirSync('./coverage', {recursive: true});
    // fs.writeFileSync('./coverage/coverage-render.json', report);
}

2, jest-unit, , using jest custom reporter `jest-monocart-coverage` and option `raw` report to export original coverage data to `./coverage-reports/unit/raw`

// jest.config.ts
const config: Config = {

    collectCoverage: true,
    coverageProvider: 'v8',
    coverageReporters: ['none'],
    reporters: [
        ['jest-monocart-coverage', {
            // logging: 'debug',
            name: 'My Unit Coverage Report',

            reports: [
                ['v8'],
                ['raw'],
                ['console-summary'],
                ['html', {
                    subdir: 'html'
                }],
                //['lcovonly']
            ],

            sourceFilter: (sourcePath) => {
                return !sourcePath.includes('node_modules/') && sourcePath.search(/src\//) !== -1;
            },

            outputDir: './coverage-reports/unit'
        }]
    ]
}

3, merge coverage with option `inputDir`

// merge-coverage.js
import {CoverageReport} from 'monocart-coverage-reports';

new CoverageReport({

    name: 'My Merged Coverage Report',

    reports: [
        ['v8'],
        ['console-summary'],
        ['codecov']
    ],

    sourceFilter: (sourcePath) => {
        return !sourcePath.includes('node_modules/') && sourcePath.search(/src\//) !== -1;
    },

    inputDir: [
        './coverage-reports/render/raw',
        './coverage-reports/unit/raw'
    ],

    outputDir: './coverage-reports/merged'
}).generate();

Add it to npm scripts

{
  scripts: {
    "merge-coverage": "node ./merge-coverage.js",
  }
}

Please have a try. Thanks.

HarelM commented 8 months ago

The merge is done in codecov automaticall, but I'll look into adding this. Do I need all these report types? Also currently, only the CI adds the coverage option and running locally you'll get only the code results. Is there a way to add this reported in the command line? I suppose not, but just wanted to verify.

cenfun commented 8 months ago

I didn't know before that Codecov can merge automatically, then the third step should not be necessary.

Is there a way to add this reported in the command line?

It does support CLI, but it might not have the command you're looking for. Could you tell me about your intended use case? Perhaps it's worth implementing.

HarelM commented 8 months ago

I run coverage only as part of my CI build here: https://github.com/maplibre/maplibre-gl-js/blob/fa7372f0493579a60bdf6ac8f1547deb3bc1bcee/.github/workflows/test-all.yml#L48 In this command line I add the relevant reporters for jest - json, text etc. So, I was hoping I could simply update this line to add this reporter. I'm guessing that these reporters are built in into jest, so this is possible for them but not for this one. But it would be great to reduce the configuration in order to run this coverage reporter.

cenfun commented 8 months ago

I understand your question now. In the command line, Jest should only be able to accept the name of the custom report, it may not be able to accept options at the next level. Therefore, you need to add the custom reporter options inside the jest.config.ts file.

HarelM commented 8 months ago

I see, I'll look into it, hopefully in the next few days and let you know if I find other places where this looks odd. Thanks for all the work on this!!

HarelM commented 8 months ago

My jest configuration has a few projects, is there a way to only configure this for a single project? See here: https://github.com/maplibre/maplibre-gl-js/blob/fa7372f0493579a60bdf6ac8f1547deb3bc1bcee/jest.config.ts#L17

HarelM commented 8 months ago

Ok, I did a quick test and here is my input:

raw folder contains a lot of files, I'm not sure who will ever use it, so I think a merged json would be better here, even one called raw.json.
When setting this report I no longer get the progress when running jest on my machine, it might also be problematic if there are logs I'm printing in order to debug some tests: This is missing:
As I said, the configuration is for all my tests, where I would like to configure this per project, I'm not sure how jest support this as it seems like it doesn't support this very well and maybe a better solution would be to split the config into different files, IDK...
I'm looking for a minimal configuration in order to generate the json file, I'm not sure I understand from the above config what I can omit and what I have to set in order for this to work.

Hope this helps!

HarelM commented 8 months ago

Another note, I was hoping I could run the tests without the coverage report in some cases and with the coverage report added as a command line flag. i.e. set collectCoverage to false (the default) and run only in CI npm run test-unit -- --coverage When running without the coverage I get the following message: Determining test suites to run...No v8 coverage data collected And I don't know if the tests passed or not. I think this is similar to item (2) in the list above.

cenfun commented 8 months ago

Just released a new version jest-monocart-coverage@1.0.2 try setting collectCoverage to false by default in jest.config.ts, and run with -- --coverage to enable the coverage report

HarelM commented 8 months ago

Cool! It doesn't fail, but it doesn't show the number of failed tests. Determining test suites to run...% Let me get a branch up so you can experiment on my project.

cenfun commented 8 months ago

1, raw folder contains a lot of files yes, it should be removed if useless after merged.

When setting this report I no longer get the progress when running jest on my machine try adding a default report:
```
reporters: [
    'default',
    ['jest-monocart-coverage', {}]
    ]
```

HarelM commented 8 months ago

The above link is from a PR I just opened to see how things behave: https://github.com/maplibre/maplibre-gl-js/pull/3615 I think a merged raw folder json is better instead of everyone writing and running the merge code, even if you don't merge from other tests frameworks like I do...

HarelM commented 8 months ago

Thanks, default solves the progress issue.

cenfun commented 8 months ago

Simply removing the raw files onEnd

import fs from "fs";
import {CoverageReport} from 'monocart-coverage-reports';

const inputDir = [
    './coverage-reports/render/raw',
    './coverage-reports/unit/raw'
];

new CoverageReport({

    name: 'My Merged Coverage Report',

    reports: [
        ['v8'],
        ['console-summary'],
        ['codecov']
    ],

    sourceFilter: (sourcePath) => {
        return !sourcePath.includes('node_modules/') && sourcePath.search(/src\//) !== -1;
    },

    inputDir,

    outputDir: './coverage-reports/merged',

    onEnd: () => {
        inputDir.forEach(dir => {
            fs.rmSync(dir, {
                recursive: true,
                force: true
            })
        });
    }

}).generate();

HarelM commented 8 months ago

My problem is not that it creates the folder, my problem is that I need to add the above code to my repo, which I think should be intergrated into the jest reporter package. Also it seems that my CI code doesn't generate the raw folder. The following is the command: jest --reporters=github-actions --reporters=summary --reporters=jest-monocart-coverage --coverage --selectProjects unit Seems like if I add additional reporters, the reporter configured in the config file is ignored?

cenfun commented 8 months ago

Yes, I think the CLI option --reporters=xxx will overrides the option reporters in config file.

HarelM commented 8 months ago

Hmm... the github-actions reporter is a nice addition to the PR to see where tests failed, I would like to keep it if possible. Do you know what I can do to solve this?

cenfun commented 8 months ago

try create another config like jest.config.coverage.ts and using CLI like jest --config=path-to/jest.config.coverage.ts everything can defined in jest.config.coverage.ts

HarelM commented 8 months ago

Yea, I did something similar. thanks.

HarelM commented 8 months ago

Interestingly, I think the coverage for unit tests was lower by 3%, but the total was up by 3%. I'll need to find some examples to see where the difference is.

HarelM commented 8 months ago

Any chance to get a new version so I won't need to add the merge code to my repo?

cenfun commented 8 months ago

You can try using 'inputDir' to merge the raw from unit test during the render test. It requires unit test to be completed before render test. (The reason for splitting into 2 raw files and then merging them is to have the opportunity to separately view the coverage of the two parts individually on the local)

render test options:

const coverageReport = new CoverageReport({
        name: 'MapLibre Coverage Report',
        inputDir: './coverage/unit/raw',
        outputDir: './coverage/render',
        reports: [['v8'], ['codecov']]
    });

cenfun / monocart-coverage-reports