Exclude browser tests from upstream that are known to fail in Brave #7953
Conversation
| if (is_chromeos_ash) { | ||
| deps += [ "//ash/public/cpp/external_arc:external_arc" ] | ||
| } | ||
| + sources -= brave_chromium_browser_test_exclusions_payments |
There was a problem hiding this comment.
same as other PR, can we just use chromium_src overrides to effectively remove these files?
There was a problem hiding this comment.
As mentioned in #7901 (comment), that would imply emptying quite a bunch of files but, in the other hand, I think would avoid having to maintain all the ifs I had to use in the browser_tests_exclusions.gni file, so worth a go.
I'll do this change once I finish doing the same for unit_tests, thanks!
|
@bridiver As mentioned in #7901, I finally got your review comments addressed here and got to a point where all the tests that I run are passing, so I think it's a good time to ping you now. One important difference here, though, is that I couldn't find a way to "empty" JavaScript files via chromium_src overrides in a similar way to what we do for C++ files, meaning that I ended up simply disabling all JS tests (via In any case, and similar to what I posted in #7901, here is how things changed now that we're emptying files via overrides compared to the previous approach of removing them via GN files, these are the numbers: BEFORE addressing your review comments (i.e. removing failing test suites via GN):
In other words:
AFTER addressing your review comments (i.e. removing failing test suites via chromium_src overrides + ignoring ALL JS tests via include_js_tests.gni):
In other words:
As you can see, results here vary wildly and we move from running and passing 53.23% of all the tests that would pass if no exclusions were made to passing only 28.36%, which is nearly half of them. IMHO, this huge reduction is mainly caused due to the fact that we're now excluding all JS tests (instead of just a few ones) combined with the fact that I had to also exclude a bunch of C++ tests more to workaround the steep increase in crashes observed between this two versions of the PR (initially 555 crashes, now 1593!), but still I'm not sure if that justifies going for the more costly approach to exclude tests using GN rules. Perhaps this override-based method, while yielding much worse results, is still good enough as a first step towards running upstream's browser_tests though. Or perhaps I should spend some more time trying to find a better way to more selectively exclude JS tests, as well as properly understanding why exactly we have way more crashing tests now in case there's something fixable that is introducing too much noise here. @bridiver WDYT? |
|
@bridiver Thanks for the review, landing now then. |
This patch disables entire *_browsertest.cc files by making them empty
via chromium_src overrides, as well as all the JS tests via a patch to
include_js_tests.gni, so that we avoid running any test that we know
are going to fail because of the many ways in which Brave is different
than Chromium upstream.
Note that it's possible that this way of excluding tests is a bit too
agressive (i.e. it's likely that some browser tests inside the excluded
files would pass), but for now it's a good enough initial approach as
it enables us to considerably increase test coverage without having
to maintain patches in a too intrusive way.
As a reference, a this time of this patch's writing (on top of Brave
1.23.30 / Chromium 89.0.4389.86), this are the stats when running
upstream's unit tests on a Linux/Debug build, without this patch:
* Total run: 13355 tests
* Passed: 10427 tests
* Not passed: 2928 tests
- Failed: 870 tests
- Crashed: 1593 tests
- Timed out: 465 tests
With this patch applied, the numbers look like this:
* Total run: 2957 tests
* Passed: 2957 tests
* Not passed: 0 tests
That is, what we have with this patch applied looks as follows:
* 2957/13355 -> 22.14% of ALL the original tests being run
* 2957/10427 -> 28.36% of the PASSING TESTS still being run
In other words, we're increasing test coverage in 2957 browser tests
in a relatively clean way (i.e. no complex patching) while losing
71.64% of the browser tests that would run and pass if we were not
excluding them in such an agressive way. Not as good results as for
the case of unit tests, but still an improvement for test coverage.
Fix brave/brave-browser#8297
Actually, I've just rebased this and will wait for the bots again before merging because there were failures on nearly all of them and, while it's likely they were unrelated, I'd rather play it safer and give it another round before merging. |
Now merging for real |
Similarly to what we've done earlier in the year for other tests [1], this patch disables entire *_browsertest.cc files by making them empty via chromium_src overrides, as well as all the JS tests via a patch to include_js_tests.gni, so that we avoid running any test that we know are going to fail because of the many ways in which Brave is different than Chromium upstream. [1] #7953
Similarly to what we've done earlier in the year for other tests [1], this patch disables entire *_browsertest.cc files by making them empty via chromium_src overrides, as well as all the JS tests via a patch to include_js_tests.gni, so that we avoid running any test that we know are going to fail because of the many ways in which Brave is different than Chromium upstream. [1] #7953
Similarly to what we've done earlier in the year for other tests [1], this patch disables entire *_browsertest.cc files by making them empty via chromium_src overrides, as well as all the JS tests via a patch to include_js_tests.gni, so that we avoid running any test that we know are going to fail because of the many ways in which Brave is different than Chromium upstream. [1] #7953
This patch disables entire *_browsertest.cc files by making them empty
via chromium_src overrides, as well as all the JS tests via a patch to
include_js_tests.gni, so that we avoid running any test that we know
are going to fail because of the many ways in which Brave is different
than Chromium upstream.
Note that it's possible that this way of excluding tests is a bit too
agressive (i.e. it's likely that some browser tests inside the excluded
files would pass), but for now it's a good enough initial approach as
it enables us to considerably increase test coverage without having
to maintain patches in a too intrusive way.
As a reference, a this time of this patch's writing (on top of Brave
1.23.30 / Chromium 89.0.4389.86), this are the stats when running
upstream's unit tests on a Linux/Debug build, without this patch:
With this patch applied, the numbers look like this:
That is, what we have with this patch applied looks as follows:
In other words, we're increasing test coverage in 2957 browser tests
in a relatively clean way (i.e. no complex patching) while losing
71.64% of the browser tests that would run and pass if we were not
excluding them in such an agressive way. Not as good results as for
the case of unit tests, but still an improvement for test coverage.
Fix brave/brave-browser#8297
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Run all the enabled browser tests from upstream with npm run test -- browser_tests and check that they all pass