Another way to achieve bug 1472737 is to make the baseline test that is used for browser-chrome tests use the functions from BrowserTestUtils. This way we collect coverage for them in the baseline and we remove it from the actual tests. Greg, could you take this?

This should only be done for the baseline used for browser-chrome, otherwise we will remove useful coverage from other tests.

Yup, I can do this. I'm leaving the ni? on for myself.

Here's a test run for the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18d2b6f9194bb8b521a0f39577b029fd7b42c155

Comment on attachment 8995354 [details] Bug 1476526 - Make browser-chrome baseline test use functions from BrowserTestUtils. https://reviewboard.mozilla.org/r/259810/#review266946 The patch to add some BrowserTestUtils calls looks good (there are some other functions we might use, but they are way less important than the ones you added, and we don't want to make the baseline test huge unless really necessary), but there's a problem: we're using the browser-chrome baseline as a baseline also for non-browser-chrome tests, so this means we might remove useful coverage from other tests. I see two possible solutions here: 1) Create two separate baseline tests, one without these changes that you keep using for all tests ending with ".js" and one with these changes that you only use for browser-chrome tests; 2) Select the baseline from the test suite/flavour rather than the extension. I would prefer 2 as I feel it's cleaner, but I'm OK with 1 if it's faster to implement and then we can do 2 in a follow-up.

I share your concerns, but I am also worried that this will negatively impact browser-chrome test coverage in cases like [1]. What do you think? Or, why do you think other suites would have problems but not browser-chrome? I agree that (2) would be the ideal solution, but it would take a bit of time to implement that considering that we would have to add at-least 1 test per suite. I can do (1) though quickly. [1]: https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/events/browser_test_focus_dialog.js#24

(In reply to Greg Mierzwinski [:sparky] from comment #6) > I share your concerns, but I am also worried that this will negatively > impact browser-chrome test coverage in cases like [1]. What do you think? > Or, why do you think other suites would have problems but not browser-chrome? > > I agree that (2) would be the ideal solution, but it would take a bit of > time to implement that considering that we would have to add at-least 1 test > per suite. I can do (1) though quickly. > > > [1]: > https://dxr.mozilla.org/mozilla-central/source/accessible/tests/browser/ > events/browser_test_focus_dialog.js#24 The browser-chrome tests are (at least usually) using those functions as utility functions, but they are not actually testing what they do. Other test suites are a little lower level, so they might be testing subsets of the things done by those functions (e.g. window opening). 1 could also be a first step to achieve 2: you could have a first map that goes from test suite / flavor to baseline, if the test suite / flavour is in the map you use that, otherwise you fallback to the extension.

Ah ok, I understand now. I'll start implementing (1) as you suggest so that it will lead into (2).

I've got this working on a per-suite basis here (preparing for the future work as well): https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5b1140cf2fc0ec2a0cb7ddf176f60b03834312 I'm just running one more test here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b76094f56ac84c5d010e4181a2e3f4e63d019c8

Comment on attachment 8997194 [details] Bug 1476526 - Make the baseline test use functions from BrowserTestUtils. https://reviewboard.mozilla.org/r/261094/#review268228 Looks good to me! Just a bunch of minor nits. ::: testing/mochitest/baselinecoverage/browser_chrome/browser_baselinecoverage_browserchrome.js:17 (Diff revision 1) > + > + await BrowserTestUtils.withNewTab({ > + gBrowser, > + url: "about:blank" > + }, async function(browser) { > + ok(true, "Collecting baseline coverage for javascript (.js) file types."); Nit: this comment should be updated now :) ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:173 (Diff revision 1) > 'suite': 'chrome' > } > } > > + baseline_tests_by_suite = { > + 'browser-chrome': 'testing/mochitest/baselinecoverage/browser_chrome/' + Nit: no need for the string concatenation ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:379 (Diff revision 1) > if not self.per_test_reports: > self.info("No tests were found...not saving coverage data.") > return > > # Get the baseline tests that were run. > baseline_tests_cov = {} Nit: maybe we can rename this `baseline_tests_ext_cov`? To avoid confusion. ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:392 (Diff revision 1) > # Bug 1460064 is filed for this. > - _, baseline_filetype = os.path.splitext(test) > with open(grcov_file, 'r') as f: > - baseline_tests_cov[baseline_filetype] = json.load(f) > + data = json.load(f) > + > + if suite.replace('-', '') in test: Nit: if you rename the test file browser_baselinecoverage_browser-chrome.js you won't need to do the replacing here. We could keep the same convention for all the other baseline tests we're going to add in the future. ::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:419 (Diff revision 1) > > # Get baseline coverage > baseline_coverage = {} > if self.config.get('per_test_category') == "web-platform": > baseline_coverage = baseline_tests_cov['.html'] > + elif suite in baseline_tests_suite_cov: Nit: I'd move this case first, as it's the only one we'll eventually have

And here's a test run for the latest changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4485ee588442f79239466459dafd3bdd8a90424

Comment on attachment 8997194 [details] Bug 1476526 - Make the baseline test use functions from BrowserTestUtils. https://reviewboard.mozilla.org/r/261094/#review268252

Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/47aef65bc40d Make the baseline test use functions from BrowserTestUtils. r=marco