IsCodecSupportedOnAndroidTest is not actually testing all combinations of PlatformInfo settings
Reported by
pa...@yandex-team.ru,
Apr 7 2017
|
|
Issue description
There is an issue in implementation of unit test media_unittests.IsCodecSupportedOnAndroidTest:
The parameterized template function RunCodecSupportTest is supposed to invoke tested callback with different combinations of PlatformInfo settings, but it actually uses only initial values of PlatformInfo fields.
Detailed explanation:
Inside implementation of
template <typename TestCallback>
static void RunCodecSupportTest(const MimeUtil::PlatformInfo& states_to_vary,
const MimeUtil::PlatformInfo& test_states,
TestCallback test_func)
There is a macro definition
#define RUN_TEST_VECTOR(name) \
size_t name##_index = 0; \
for (info.name = name##_states[name##_index]; \
name##_index < name##_states.size(); ++name##_index)
Where |info| is a local variable MimeUtil::PlatformInfo info;
This particular for loop assigns a value to info.<name> field only in its initialization part (i.e. for definition has 3 parts: for(<initializaton>; <condition>; <post-iteration-step>) ). Obiously it will assign the field of |info| only before first iteration and then the field remains unchanged no matter what |index| we are iterating at.
The correct implementation is the following:
#define RUN_TEST_VECTOR(name) \
for (size_t name##_index = 0; \
info.name = name##_states[name##_index], \
name##_index < name##_states.size(); ++name##_index)
Here we assign a value to info.name every iteration so it goes through all the values in states vector.
As I fixed this issue it turned out that test IsCodecSupportedOnAndroidTest.ClearCodecBehavior starts to fail, because previously it was only testing the return value of IsCodecSupportedOnAndroid only with PlatformInfo.has_platform_decoders = true, and not with PlatformInfo.has_platform_decoders = false, even though it should actually vary all fields, as MimeUtil::PlatformInfo states_to_vary = VaryAllFields();
Looking deeper it seems that H264 is always supported for non-encrypted video, but the test supposes, that H264 support depends on PlatformInfo.has_platform_decoders and is not available when PlatformInfo.has_platform_decoders = false.
I will provide the appropriate CL that fixes all described issues.
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b75c4d80c9ac27428c760c727c39500d6e8c27b commit 1b75c4d80c9ac27428c760c727c39500d6e8c27b Author: pavor <pavor@yandex-team.ru> Date: Mon Apr 10 10:29:26 2017 Fix PlatformInfo variation for mime util IsCodecSupportedOnAndroidTest The parameterized template function RunCodecSupportTest is supposed to invoke tested callback with different combinations of PlatformInfo settings, but it actually used only initial values of PlatformInfo fields. Also it seems that H264 is always supported for non-encrypted video, so the test IsCodecSupportedOnAndroidTest.ClearCodecBehavior should be corrected. This has been revealed only after fixing parameter variation. R=chcunningham, dalecurtis BUG=709428 Review-Url: https://codereview.chromium.org/2804883007 Cr-Commit-Position: refs/heads/master@{#463213} [modify] https://crrev.com/1b75c4d80c9ac27428c760c727c39500d6e8c27b/media/base/mime_util_unittest.cc
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8172a213e56ea4862d5337c975e5d3c7ad50dfe commit e8172a213e56ea4862d5337c975e5d3c7ad50dfe Author: mkwst <mkwst@chromium.org> Date: Mon Apr 10 12:19:20 2017 Revert of Fix PlatformInfo variation for mime util IsCodecSupportedOnAndroidTest (patchset #1 id:1 of https://codereview.chromium.org/2804883007/ ) Reason for revert: Broke IsCodecSupportedOnAndroidTest on https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/24644. Original issue's description: > Fix PlatformInfo variation for mime util IsCodecSupportedOnAndroidTest > > The parameterized template function RunCodecSupportTest is supposed to > invoke tested callback with different combinations of PlatformInfo settings, > but it actually used only initial values of PlatformInfo fields. > > Also it seems that H264 is always supported for non-encrypted video, so the > test IsCodecSupportedOnAndroidTest.ClearCodecBehavior should be corrected. > This has been revealed only after fixing parameter variation. > > R=chcunningham, dalecurtis > BUG=709428 > > Review-Url: https://codereview.chromium.org/2804883007 > Cr-Commit-Position: refs/heads/master@{#463213} > Committed: https://chromium.googlesource.com/chromium/src/+/1b75c4d80c9ac27428c760c727c39500d6e8c27b TBR=chcunningham@chromium.org,dalecurtis@chromium.org,pavor@yandex-team.ru # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=709428 Review-Url: https://codereview.chromium.org/2811693003 Cr-Commit-Position: refs/heads/master@{#463229} [modify] https://crrev.com/e8172a213e56ea4862d5337c975e5d3c7ad50dfe/media/base/mime_util_unittest.cc
,
Apr 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56a00719145e7f3f690cbeabb6025e5420932782 commit 56a00719145e7f3f690cbeabb6025e5420932782 Author: pavor <pavor@yandex-team.ru> Date: Wed Apr 12 09:03:32 2017 Fix PlatformInfo variation for mime util IsCodecSupportedOnAndroidTest The parameterized template function RunCodecSupportTest is supposed to invoke tested callback with different combinations of PlatformInfo settings, but it actually used only initial values of PlatformInfo fields. Also it seems that H264 is always supported for non-encrypted video, so the test IsCodecSupportedOnAndroidTest.ClearCodecBehavior should be corrected. This has been revealed only after fixing parameter variation. Initial changeset https://codereview.chromium.org/2804883007 was reverted, I was accessing a vector at index just before checking the index vs vector size. This time I moved access of the states vector into loop body, and split RUN_TEST_VECTOR into two macros: RUN_TEST_VECTOR_BEGIN and RUN_TEST_VECTOR_END. R=chcunningham, dalecurtis BUG=709428 Review-Url: https://codereview.chromium.org/2808373002 Cr-Commit-Position: refs/heads/master@{#463968} [modify] https://crrev.com/56a00719145e7f3f690cbeabb6025e5420932782/media/base/mime_util_unittest.cc |
|
►
Sign in to add a comment |
|
Comment 1 by ppolise...@chromium.org
, Apr 7 2017Owner: qin...@chromium.org
Status: Assigned (was: Unconfirmed)