test launcher: make --gtest_filter=A.B + --gtest_also_run_disabled_tests also run A.DISABLED_B or D…

test launcher: make --gtest_filter=A.B + --gtest_also_run_disabled_tests also run A.DISABLED_B or DISABLED_A.B or DISABLED_A.DISABLED_B

BUG=658892
Committed: https://crrev.com/9537aa9f56cc489b48b43bf638dda3968c28993d
Cr-Commit-Position: refs/heads/master@{#428827}

[stgao, 2016-10-24 21:44:46]

stgao@chromium.org changed reviewers:
+ estaab@chromium.org, phajdan.jr@chromium.org

[stgao, 2016-10-24 21:48:23]

https://codereview.chromium.org/2439423003/diff/1/base/test/gtest_util.cc
File base/test/gtest_util.cc (right):

https://codereview.chromium.org/2439423003/diff/1/base/test/gtest_util.cc#new...
base/test/gtest_util.cc:31: ReplaceSubstringsAfterOffset(&test_name_no_disabled,
0, "DISABLED_", "");
For test A.B, if A or B matches either "DISABLED_*" or "*/DISABLED_*", A.B is
treated as a disabled test.

Do we want to do the same here? Do we really care about test names like
"AnEnabledTest.ButTestWithDISABLED_Feature"?

[Paweł Hajdan Jr., 2016-10-25 18:17:00]

LGTM w/comments

https://codereview.chromium.org/2439423003/diff/1/base/test/gtest_util.cc
File base/test/gtest_util.cc (right):

https://codereview.chromium.org/2439423003/diff/1/base/test/gtest_util.cc#new...
base/test/gtest_util.cc:31: ReplaceSubstringsAfterOffset(&test_name_no_disabled,
0, "DISABLED_", "");
On 2016/10/24 21:48:23, stgao wrote:
> For test A.B, if A or B matches either "DISABLED_*" or "*/DISABLED_*", A.B is
> treated as a disabled test.
> 
> Do we want to do the same here? Do we really care about test names like
> "AnEnabledTest.ButTestWithDISABLED_Feature"?

The code is fine. It matches existing implementation of
TestNameWithoutDisabledPrefix .

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_lau...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_lau...
base/test/launcher/test_launcher.cc:977: std::string normlized_test_name =
NormalizeTestName(test_name);
nit: normlized -> normalized (typo)

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_res...
File base/test/launcher/test_results_tracker.cc (left):

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_res...
base/test/launcher/test_results_tracker.cc:32: std::string
TestNameWithoutDisabledPrefix(const std::string& test_name) {
Why not use TestNameWithoutDisabledPrefix as a name? It's very specific, while
"normalize" doesn't have a clear meaning in that context.

[stgao, 2016-10-25 19:13:28]

Patchset #2 (id:20001) has been deleted

[stgao, 2016-10-25 19:16:49]

https://codereview.chromium.org/2439423003/diff/1/base/test/gtest_util.cc
File base/test/gtest_util.cc (right):

https://codereview.chromium.org/2439423003/diff/1/base/test/gtest_util.cc#new...
base/test/gtest_util.cc:31: ReplaceSubstringsAfterOffset(&test_name_no_disabled,
0, "DISABLED_", "");
On 2016/10/25 18:16:59, Paweł Hajdan Jr. wrote:
> On 2016/10/24 21:48:23, stgao wrote:
> > For test A.B, if A or B matches either "DISABLED_*" or "*/DISABLED_*", A.B
is
> > treated as a disabled test.
> > 
> > Do we want to do the same here? Do we really care about test names like
> > "AnEnabledTest.ButTestWithDISABLED_Feature"?
> 
> The code is fine. It matches existing implementation of
> TestNameWithoutDisabledPrefix .

Acknowledged.

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_lau...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_lau...
base/test/launcher/test_launcher.cc:977: std::string normlized_test_name =
NormalizeTestName(test_name);
On 2016/10/25 18:16:59, Paweł Hajdan Jr. wrote:
> nit: normlized -> normalized (typo)

Done.

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_res...
File base/test/launcher/test_results_tracker.cc (left):

https://codereview.chromium.org/2439423003/diff/1/base/test/launcher/test_res...
base/test/launcher/test_results_tracker.cc:32: std::string
TestNameWithoutDisabledPrefix(const std::string& test_name) {
On 2016/10/25 18:16:59, Paweł Hajdan Jr. wrote:
> Why not use TestNameWithoutDisabledPrefix as a name? It's very specific, while
> "normalize" doesn't have a clear meaning in that context.

Was to follow the existing function "FormatFullTestName" in gtest_util.h to find
a verb :)
Now reverted to use TestNameWithoutDisabledPrefix instead.

[stgao, 2016-10-31 20:05:15]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-10-31 20:05:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2439423003/#ps60001
(title: "Just rebase.")

[commit-bot: I haz the power, 2016-10-31 20:05:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 21:39:11]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-31 21:41:00]

Description was changed from

==========
test launcher: make --gtest_filter=A.B + --gtest_also_run_disabled_tests also
run A.DISABLED_B or DISABLED_A.B or DISABLED_A.DISABLED_B

BUG=658892
==========

to

==========
test launcher: make --gtest_filter=A.B + --gtest_also_run_disabled_tests also
run A.DISABLED_B or DISABLED_A.B or DISABLED_A.DISABLED_B

BUG=658892

Committed: https://crrev.com/9537aa9f56cc489b48b43bf638dda3968c28993d
Cr-Commit-Position: refs/heads/master@{#428827}
==========

[commit-bot: I haz the power, 2016-10-31 21:41:01]

Patchset 3 (id:??) landed as
https://crrev.com/9537aa9f56cc489b48b43bf638dda3968c28993d
Cr-Commit-Position: refs/heads/master@{#428827}