Consolidate media unittests |
||||
Issue description
Today under media/ we have a lot of unittests:
media_unittests
media_blink_unittests
capture_unittests
media_mojo_unittests
media_remoting_unittests
midi_unittests
This provides nice separation on test coverage, but on the other hand, we have to manually add each to test/try bots, which seems unfortunate.
It would be nice if we have one big test target that runs all tests under media/ (e.g. media_unittests) which we can enable on bots, but we still have smaller test targets to run locally (e.g. media_base_unittests, media_mojo_unittests).
Maybe we can do something like this:
source_set(media_base_test) {
testonly = true
sources = [
"media_base_unittest.cc",
......
]
}
test(media_foo_unittests) {
"run_all_tests.cc",
deps = [
":media_base_test",
]
}
test(media_unittests) {
"run_all_tests.cc",
deps = [
":media_base_test",
":media_mojo_test",
":capture_test",
....
]
}
One problem with the approach is that currently we have multiple run_all_unittests.cc which do different initialization. For example, blink/mojo all require some special initialization logic. We would need a way to
consolidate them somehow.
Comments?
,
Oct 18 2016
If there are no objections, I'll start moving things around a bit so media_remoting_unittests can be merged in. We'll gradually merge other tests as needed. Re#1: Yeah, for mojo, it's the EDK call. For blink, see https://cs.chromium.org/chromium/src/media/blink/run_all_unittests.cc?rcl=0&l=27
,
Oct 18 2016
On #1: mojo is so pervasive now, that it doesn't make sense to have specific run_all_unittests.cc with and without it (essentially all of them do have it now: https://cs.chromium.org/search/?q=run_all_unittests+mojo::edk::Init&sq=package:chromium&type=cs). One of the reasons for the multiplicity is that media/ is composed of a big blob of "stuff", including and calling each other freely, and a number of satellites that were too small to become top level folders, but didn't want to be eaten into the blob. The root cause is semantics: "media" doesn't really define anything, so the area has become a parking lot where things in anyway related to video and audio end up and, as we try to slice and group areas together, we fail to find commonalities. The current Chrome structure is more nuanced than when //media was hatched, and we should think about moving stuff to //devices //services, and //components instead of shuffling things around.
,
Oct 18 2016
Also, it seems in chrome we usually use "unit_tests" as the source_set name for unit test files [1], but in media we use "unitttests" [2]. I think we (media) should be consistent. [1] https://cs.chromium.org/search/?q=source_set%5C(%5C%22unit_test&sq=package:chromium&type=cs [2] https://cs.chromium.org/search/?q=source_set%5C(%5C%22unittest&sq=package:chromium&type=cs
,
Oct 18 2016
Re #3: Partly agreed that media/ has been a parking lot for too many stuff, and some of them probably should have been moved to //services, or //components. But there are definitely folders that are close to the core media playback that should stay, e.g. media/mojo, media/remoting, media/blink. For what'll stay in media/, we should provide a unified test target so that we don't need to put every little test in the test config files.
,
Oct 18 2016
,
Oct 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f072c4c56b530b857771e3eea4358ba0daea3c19 commit f072c4c56b530b857771e3eea4358ba0daea3c19 Author: xhwang <xhwang@chromium.org> Date: Fri Oct 21 00:53:38 2016 media: Reorganize media unittests - Put the current "media" unittest files in a source_set. - Add a "media_base_unittests" target which includes only "media" unittests. - Update "media_unittests" so that it includes "media" unittests for now but in the future it will also include tests for other subfolders, e.g. caputre, remoting, mojo etc. - Also rename "unittests" targets to "unit_tests" to be consistent to the rest of the codebase. BUG= 656112 TEST=This only reorganizes tests and doesn't change any existing tests. Review-Url: https://chromiumcodereview.appspot.com/2426773005 Cr-Commit-Position: refs/heads/master@{#426665} [modify] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/BUILD.gn [modify] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/audio/BUILD.gn [modify] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/base/BUILD.gn [modify] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/base/android/BUILD.gn [modify] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/base/mac/BUILD.gn [modify] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/test/BUILD.gn [rename] https://crrev.com/f072c4c56b530b857771e3eea4358ba0daea3c19/media/test/run_all_unittests.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0f8651858496747ff39236c22ec64a7b5037117 commit b0f8651858496747ff39236c22ec64a7b5037117 Author: xhwang <xhwang@chromium.org> Date: Thu Oct 27 18:35:23 2016 media: media_unittests also run media mojo unittests Add media mojo unittests to media_unittets target, so that on bots we don't need to specify multiple smaller media unittests targets. This reverts commit 55b05c7a4c6722911ee2c715ed3a519da8d12315 and partially reverts commit f69024d1ddb7ee6b001203b13a019b6fc9dc7746. "media_mojo_unittests" target is still available to be built and run locally. BUG= 656112 Review-Url: https://codereview.chromium.org/2455553002 Cr-Commit-Position: refs/heads/master@{#428088} [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/media/BUILD.gn [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/media/mojo/BUILD.gn [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.chromiumos.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.full.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.linux.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.mac.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.memory.full.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.memory.fyi.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.memory.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium.win.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/chromium_memory_trybot.json [modify] https://crrev.com/b0f8651858496747ff39236c22ec64a7b5037117/testing/buildbot/gn_isolate_map.pyl
,
Oct 27 2016
We still have many test targets in media/ that we can reorganize, but I don't have any plan to do it any time soon. I'll close this issue and we can file new bugs for further cleanup work.
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d256f62161cf485d02752780ae74b4704f7d8d3 commit 6d256f62161cf485d02752780ae74b4704f7d8d3 Author: xhwang <xhwang@chromium.org> Date: Thu Dec 01 23:42:18 2016 media: Fix media_mojo_shell_unittests This test is very broken after various mojo/media changes. Today it's passing on some fyi bots only because we are lucky. This CL fixes the tests so that it's passing locally. TODO in later CLs: - The naming needs to be updated as well - Merge this test into media_mojo_tests BUG= 656112 TEST=Fix existing tests. Review-Url: https://codereview.chromium.org/2546633002 Cr-Commit-Position: refs/heads/master@{#435764} [modify] https://crrev.com/6d256f62161cf485d02752780ae74b4704f7d8d3/media/mojo/BUILD.gn [modify] https://crrev.com/6d256f62161cf485d02752780ae74b4704f7d8d3/media/mojo/services/BUILD.gn [delete] https://crrev.com/902df7e81d1c9f04e1500dd0bce59647b08d6b85/media/mojo/services/apptest_manifest.json [modify] https://crrev.com/6d256f62161cf485d02752780ae74b4704f7d8d3/media/mojo/services/media_manifest.json [modify] https://crrev.com/6d256f62161cf485d02752780ae74b4704f7d8d3/media/mojo/services/media_mojo_unittest.cc [modify] https://crrev.com/6d256f62161cf485d02752780ae74b4704f7d8d3/media/mojo/services/test_manifest.json
,
Dec 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9c958fd2553179870953b398506678a5e1319714 commit 9c958fd2553179870953b398506678a5e1319714 Author: xhwang <xhwang@chromium.org> Date: Mon Dec 05 23:17:16 2016 media: Rename media_mojo_shell_unittests to media_service_unittests TBR=jam@chromium.org BUG= 656112 TEST=No functionality change Review-Url: https://codereview.chromium.org/2551963002 Cr-Commit-Position: refs/heads/master@{#436442} [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/BUILD.gn [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/media/mojo/clients/BUILD.gn [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/media/mojo/services/BUILD.gn [rename] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/media/mojo/services/media_service_unittest.cc [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/media/mojo/services/test_manifest.json [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/testing/buildbot/chromium.linux.json [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/testing/buildbot/gn_isolate_map.pyl [modify] https://crrev.com/9c958fd2553179870953b398506678a5e1319714/tools/determinism/deterministic_build_whitelist.pyl |
||||
►
Sign in to add a comment |
||||
Comment 1 by m...@chromium.org
, Oct 18 2016