-
Notifications
You must be signed in to change notification settings - Fork 929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exclude unit tests from upstream that are known to fail in Brave #7901
Conversation
Note: Creating a draft PR for now, since this PR depends on #7899 being merged first, whose only commit is temporarily being included here for convenience. |
553197b
to
6933502
Compare
34e81a7
to
1be944a
Compare
"providers/openscreen/network_service_quic_packet_writer_unittest.cc", | ||
] | ||
} | ||
+ sources -= brave_chromium_unit_test_exclusions_media_router |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where these are defined, but can we just remove them by using chromium_src overrides to make the file empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where these are defined
GitHub is not very nice collapsing the content of the newly added unit_tests_exclusions.gni
file, which I think is why you might have overlooked its definition, but it's indeed there. See https://github.com/brave/brave-core/pull/7901/files#diff-663d5215178a6aab5144409eee430f87810538a46612375d5e702fb0af75a8d6R276
but can we just remove them by using chromium_src overrides to make the file empty?
We could do that, but before I do so please be aware that there are many *_unittest.cc
files that are being disabled via definitions like this one from unit_tests_exclusions.gni
, so there would be quite a bunch of overrides to be made.
In the other hand, emptying those *_unittests.cc
files would prevent us from having to maintain the tedious collection of if
's I had to use in unit_tests_exclusions.gni
to make sure that we only remove a test that has been previously added according to whatever build options we're using, so it's certainly worth a go.
Will try this out now and update the PR accordingly once it's ready for another review.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7ee89bc
to
8b1cf8b
Compare
This patch disables entire *_unittests.cc files by making them empty via chromium_src overrides, 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 unit 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: 13504 tests * Passed: 11585 tests * Not passed: 1919 tests - Failed: 182 tests - Crashed: 1737 tests - Timed out: 0 tests With this patch applied, the numbers look like this: * Total run: 10045 tests * Passed: 10045 tests * Not passed: 0 tests That is, what we have with this patch applied looks as follows: * 10045/13504 -> 74.39% of ALL the original tests being run * 10045/11585 -> 86.71% of the PASSING TESTS still being run In other words, we're increasing test coverage in 10045 unit tests in a relatively clean way (i.e. no complex patching) while "only" losing 13.29% of the unit tests that would run and pass if we were not excluding them in such an agressive way. Fix brave/brave-browser#8376
@bridiver Sorry for the extremely long delay, but I had some difficulties addressing similar review comments for #7953 (for browser_tests) which delayed this, but now that I've just pushed changes to that other PR, I feel better about pinging you here again. Now, as for 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):
In other words:
Thus, results are very similar between the two approaches, although it's true this one of skipping test suites via overrides is simpler and likely easier to maintain than messing with GN files and the different conditions that need to be taken into account not to get build failures (e.g. we can't remove a files via @bridiver PTAL and let me know what you think, thanks! |
@bridiver Thanks for the review, landing now then. |
Similarly to what we've done earlier in the year for other tests [1], this patch disables entire *_unittests.cc files by making them empty via chromium_src overrides, 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] #7901
Similarly to what we've done earlier in the year for other tests [1], this patch disables entire *_unittests.cc files by making them empty via chromium_src overrides, 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] #7901
Similarly to what we've done earlier in the year for other tests [1], this patch disables entire *_unittests.cc files by making them empty via chromium_src overrides, 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] #7901
This patch disables entire *_unittests.cc files by making them empty
via chromium_src overrides, 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 unit 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 10045 unit tests
in a relatively clean way (i.e. no complex patching) while "only"
losing 13.29% of the unit tests that would run and pass if we were
not excluding them in such an agressive way.
Fix brave/brave-browser#8376
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-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 tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Run all the enabled unit tests from upstream with
npm run test -- unit_tests
and check that they all pass