Closed Bug 1476526 Opened 6 years ago Closed 6 years ago

Make the baseline test use functions from BrowserTestUtils, so that they are ignored

Categories

(Testing :: Code Coverage, enhancement)

Product:
Testing
Component:
Code Coverage
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: marco, Assigned: sparky)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1476526 - Make the baseline test use functions from BrowserTestUtils.
59 bytes, text/x-review-board-request
marco
: review+
Details 
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?
Flags: needinfo?(gmierz2)
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.
Assignee: nobody → gmierz2
Flags: needinfo?(gmierz2)
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.
Attachment #8995354 - Flags: review?(mcastelluccio)
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
Flags: needinfo?(mcastelluccio)
(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.
Flags: needinfo?(mcastelluccio)
Ah ok, I understand now. I'll start implementing (1) as you suggest so that it will lead into (2).
Blocks: 1479109
Attachment #8995354 - Attachment is obsolete: true
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
Attachment #8997194 - Flags: review?(mcastelluccio) → review+
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
https://hg.mozilla.org/mozilla-central/rev/47aef65bc40d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: