Skip to content

Collect coverage information for the integration tests#21173

Open
timvandermeij wants to merge 1 commit intomozilla:masterfrom
timvandermeij:github-actions-integration-tests-coverage
Open

Collect coverage information for the integration tests#21173
timvandermeij wants to merge 1 commit intomozilla:masterfrom
timvandermeij:github-actions-integration-tests-coverage

Conversation

@timvandermeij
Copy link
Copy Markdown
Contributor

@timvandermeij timvandermeij commented Apr 27, 2026

Note that for the integration tests the coverage information ends up being processed in the Node.js context where window is not available, so we use globalThis instead for the function that merges individual test's coverage information into the global object because that is available in all contexts we support. For clarity we also rename said function since we're not exclusively dealing with window nor worker data anymore.

@timvandermeij timvandermeij added test infra Infrastructure related labels Apr 27, 2026
@timvandermeij timvandermeij marked this pull request as draft April 27, 2026 13:30
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.54%. Comparing base (ac51bdf) to head (d6916ac).

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #21173       +/-   ##
===========================================
+ Coverage   55.92%   74.54%   +18.61%     
===========================================
  Files         220      254       +34     
  Lines       58990    64614     +5624     
===========================================
+ Hits        32993    48167    +15174     
+ Misses      25997    16447     -9550     
Flag Coverage Δ
fonttest 8.65% <ø> (ø)
integrationtest 76.18% <ø> (?)
unittest 55.19% <ø> (-0.02%) ⬇️
unittestcli 55.66% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 24e380c to 2f29315 Compare April 27, 2026 14:16
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 2f29315 to f39eb0f Compare April 27, 2026 14:21
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from f39eb0f to 68e44a4 Compare April 27, 2026 14:26
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 68e44a4 to 9976a72 Compare April 27, 2026 14:33
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from d94f5a8 to 647edd5 Compare April 27, 2026 15:34
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 647edd5 to 76fa3a3 Compare April 27, 2026 15:44
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 76fa3a3 to 42084e2 Compare April 27, 2026 15:54
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 42084e2 to 12ea70d Compare April 27, 2026 16:42
@timvandermeij timvandermeij force-pushed the github-actions-integration-tests-coverage branch from 12ea70d to 4523554 Compare April 27, 2026 16:46
@Snuffleupagus
Copy link
Copy Markdown
Collaborator

Possibly a stupid idea: Given how slow the Windows/Chrome integration-tests become, how about keeping that particular combination running without coverage?

@calixteman
Copy link
Copy Markdown
Contributor

Possibly a stupid idea: Given how slow the Windows/Chrome integration-tests become, how about keeping that particular combination running without coverage?

I'm not opposed.
But in the mean time having them could help to us to have some hints on what can be improved and maybe this combination could highlight some bottlenecks.

@Snuffleupagus
Copy link
Copy Markdown
Collaborator

The failing integration-test actually looks like a pre-existing intermittent failure, given that the very same test just failed in PR #21214 for the same OS/browser combination.

Maybe that test should be marked as pending in the affected environment, and a tracking issue opened, such that this nice PR can finally land?

@timvandermeij
Copy link
Copy Markdown
Contributor Author

timvandermeij commented May 3, 2026

Thank you for reporting this. I have done some digging into this intermittent and created #21217 for it: fortunately it's reproducible locally and it's clear what the problem is (duplicated text layer content for page editing operations), but I haven't been able to figure out how that can happen yet.

In any case, it's not a blocker for this PR because it's a pre-existing issue. This PR is mostly ready, but there are a few things left I need to fix before putting this up for review.

Note that for the integration tests the coverage information ends up
being processed in the Node.js context where `window` is not available,
so we use `globalThis` instead for the function that merges individual
test's coverage information into the global object because that is
available in all contexts we support. For clarity we also rename said
function since we're not exclusively dealing with `window` nor worker
data anymore.
@timvandermeij
Copy link
Copy Markdown
Contributor Author

timvandermeij commented May 5, 2026

#21217 is fixed, so I have rebased this PR and included the final changes I still needed to make here, so it should be ready for review now.

// WebDriver BiDi protocol, otherwise Puppeteer's own serialization logic
// kicks in and that is a lot slower (see also upstream issue
// https://github.com/puppeteer/puppeteer/issues/2427).
return window.__coverage__ ? JSON.stringify(window.__coverage__) : null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You overlooked worker coverage, right ?

Copy link
Copy Markdown
Contributor Author

@timvandermeij timvandermeij May 5, 2026

Choose a reason for hiding this comment

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

Hm, good point; looks like I did. I'll have to think a bit about how to do that here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could have something like in closeSinglePage:

 const result = await page.evaluate(async () => {
    let workerCoverage = null;
    const handler =
      window.PDFViewerApplication.pdfDocument?._transport?.messageHandler;
    if (handler) {
      try {
        workerCoverage = await handler.sendWithPromise("GetWorkerCoverage", null);
      } catch {}
    }
    await window.PDFViewerApplication.testingClose();
    window.localStorage.clear();
    return {
      page: window.__coverage__ ? JSON.stringify(window.__coverage__) : null,
      worker: workerCoverage ? JSON.stringify(workerCoverage) : null,
    };
  });
  if (result.page) mergeCoverageIntoGlobal(JSON.parse(result.page));
  if (result.worker) mergeCoverageIntoGlobal(JSON.parse(result.worker));

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

Labels

infra Infrastructure related test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants