Non-SFI mode: Implement test launcher for nacl_helper_nonsfi_unittests

Non-SFI mode: Implement test launcher for nacl_helper_nonsfi_unittests.

This CL implements a test launcher customized for nacl_helper_nonsfi_unittests.
- Existing nacl_helper_nonsfi_unittests is renamed to nacl_helper_nonsfi_unittests_main.
- The new test launcher is built with a toolchain for the target architecture rather than PNaCl toolchain.
- XmlUnitTestResultPrinter is now extracted into a separate file.

TEST=Ran bots (with and without editing testing/buildbots scripts). Run locally.
BUG=358465
Committed: https://crrev.com/8fc7c823b9dd2b7890c96438f347383aa713c1d6
Cr-Commit-Position: refs/heads/master@{#333430}

[hidehiko, 2015-05-28 16:15:53]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org, phajdan.jr@chromium.org, thakis@chromium.org

[hidehiko, 2015-05-28 16:15:53]

Could you take a look?

phajdan@, base/test/...
thakis@, build/all.gyp
mseaborn@, components/nacl

Thanks,
- hidehiko

[Paweł Hajdan Jr., 2015-05-29 13:48:20]

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher.h (right):

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.h:120: void set_use_child_gtest_output(bool
value) {
This looks like a layering violation. Why is it needed?

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc:71: bool
ParseGTestList(const std::string& text,
Why not output structured list of tests like on ios and parse that?

[hidehiko, 2015-05-29 14:00:47]

Thank you for review.

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher.h (right):

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.h:120: void set_use_child_gtest_output(bool
value) {
On 2015/05/29 13:48:20, Paweł Hajdan Jr. wrote:
> This looks like a layering violation. Why is it needed?

Because nacl_helper_nonsfi_unittests_main still uses vanilla gtest library and
gtest_main, as building the test-support-library with PNaCl toolchain looks not
that straightforward due to the limited support of the toolchain, and also it
depends on some third party library, such as libxml.
So, subprocess uses xml output of vanilla gtest library with
--gtest_output=xml:PATH_TO_XML_OUTPUT, rather than linking
base::XmlUnitTestResultPrinter. Fortunately, these are compatible.

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc:71: bool
ParseGTestList(const std::string& text,
On 2015/05/29 13:48:20, Paweł Hajdan Jr. wrote:
> Why not output structured list of tests like on ios and parse that?

Because vanilla gtest does not support it.

[Paweł Hajdan Jr., 2015-06-01 10:56:59]

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher.h (right):

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.h:120: void set_use_child_gtest_output(bool
value) {
On 2015/05/29 14:00:47, hidehiko wrote:
> On 2015/05/29 13:48:20, Paweł Hajdan Jr. wrote:
> > This looks like a layering violation. Why is it needed?
> 
> Because nacl_helper_nonsfi_unittests_main still uses vanilla gtest library and
> gtest_main, as building the test-support-library with PNaCl toolchain looks
not
> that straightforward due to the limited support of the toolchain,

Is it possible to extract a smaller build target containing just the
functionality needed for nacl test launcher?

Alternatively, what is the specific problem with nacl toolchain?

> and also it depends on some third party library, such as libxml.

AFAICT that dependency comes from base/test/gtest_xml_util.cc but we only use it
for reading and not writing.

It should be possible to extract just the writing parts and only compile that
with nacl toolchain.

> So, subprocess uses xml output of vanilla gtest library with
> --gtest_output=xml:PATH_TO_XML_OUTPUT, rather than linking
> base::XmlUnitTestResultPrinter. Fortunately, these are compatible.

There are subtle differences, e.g. we've added x-teststart.

Still, this accessor just doesn't belong here. I think above recommendations
should make it unnecessary - please let me know if there's something I missed.

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc:71: bool
ParseGTestList(const std::string& text,
On 2015/05/29 14:00:47, hidehiko wrote:
> On 2015/05/29 13:48:20, Paweł Hajdan Jr. wrote:
> > Why not output structured list of tests like on ios and parse that?
> 
> Because vanilla gtest does not support it.

That's right. However, we have small utility functions for structured output,
see src/base/test/launcher/unit_test_launcher_ios.cc (e.g.
WriteCompiledInTestsToFile). How about using that? It should be possible to
extract smaller build targets that do not include bits troublesome for nacl.

[hidehiko, 2015-06-02 08:09:12]

Patchset #4 (id:60001) has been deleted

[hidehiko, 2015-06-02 08:12:17]

Thank you for review. PTAL.

Pawel, I have tried to address your comments, so as a result, I moved most of
testing utilities to base/test/... even it is only for Non-SFI NaCl unittests,
following the IOS's manner. Is it ok?

Nico, would you mind to review new base/... files, too?

Thanks,
- hidehiko

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_launcher.h (right):

https://codereview.chromium.org/1154313003/diff/20001/base/test/launcher/test...
base/test/launcher/test_launcher.h:120: void set_use_child_gtest_output(bool
value) {
On 2015/06/01 10:56:59, Paweł Hajdan Jr. wrote:
> On 2015/05/29 14:00:47, hidehiko wrote:
> > On 2015/05/29 13:48:20, Paweł Hajdan Jr. wrote:
> > > This looks like a layering violation. Why is it needed?
> > 
> > Because nacl_helper_nonsfi_unittests_main still uses vanilla gtest library
and
> > gtest_main, as building the test-support-library with PNaCl toolchain looks
> not
> > that straightforward due to the limited support of the toolchain,
> 
> Is it possible to extract a smaller build target containing just the
> functionality needed for nacl test launcher?
> 
> Alternatively, what is the specific problem with nacl toolchain?
> 
> > and also it depends on some third party library, such as libxml.
> 
> AFAICT that dependency comes from base/test/gtest_xml_util.cc but we only use
it
> for reading and not writing.
> 
> It should be possible to extract just the writing parts and only compile that
> with nacl toolchain.
> 
> > So, subprocess uses xml output of vanilla gtest library with
> > --gtest_output=xml:PATH_TO_XML_OUTPUT, rather than linking
> > base::XmlUnitTestResultPrinter. Fortunately, these are compatible.
> 
> There are subtle differences, e.g. we've added x-teststart.
> 
> Still, this accessor just doesn't belong here. I think above recommendations
> should make it unnecessary - please let me know if there's something I missed.

Ok, thank you for suggestion.
Then, how about the latest PS?

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/test_launcher_nacl_nonsfi.cc:71: bool
ParseGTestList(const std::string& text,
On 2015/06/01 10:56:59, Paweł Hajdan Jr. wrote:
> On 2015/05/29 14:00:47, hidehiko wrote:
> > On 2015/05/29 13:48:20, Paweł Hajdan Jr. wrote:
> > > Why not output structured list of tests like on ios and parse that?
> > 
> > Because vanilla gtest does not support it.
> 
> That's right. However, we have small utility functions for structured output,
> see src/base/test/launcher/unit_test_launcher_ios.cc (e.g.
> WriteCompiledInTestsToFile). How about using that? It should be possible to
> extract smaller build targets that do not include bits troublesome for nacl.

Done.

[Paweł Hajdan Jr., 2015-06-02 10:37:36]

Nice work!

I think you can now remove "Unlike other test launcher, the new test launcher
relies on the vanilla gtest a bit more. Specifically, it uses --gtest_list_tests
and --gtest_output." from commit message.

Just some minor comments.

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
File base/test/launcher/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
base/test/launcher/test_launcher_nacl_nonsfi.cc:97: if
(!base::GetAppOutput(cmd_line, &subprocess_output)) {
Why not LaunchProcess? We're ignoring subprocess_output anyway.

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
File base/test/launcher/test_launcher_nacl_nonsfi.h (right):

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
base/test/launcher/test_launcher_nacl_nonsfi.h:13: int TestLauncherMain(const
std::string& test_binary);
Could you include something related to NaCl and Non-SFI in the name of this
function? Otherwise it seems too generic.

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/run_all_unittests.cc (right):

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/run_all_unittests.cc:13: argc, argv,
base::Bind(&RUN_ALL_TESTS));
It seems a little bit dangerous to apply & directly to a macro. Could you wrap
it in a function?

[hidehiko, 2015-06-02 14:55:21]

Thank you for review. PTAL.

Note: failing tests looks unrelated to me.

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
File base/test/launcher/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
base/test/launcher/test_launcher_nacl_nonsfi.cc:97: if
(!base::GetAppOutput(cmd_line, &subprocess_output)) {
On 2015/06/02 10:37:36, Paweł Hajdan Jr. wrote:
> Why not LaunchProcess? We're ignoring subprocess_output anyway.

I thought GetAppOutput is slightly simpler as it hides the subprocess state
management. Anyway, done.

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
File base/test/launcher/test_launcher_nacl_nonsfi.h (right):

https://codereview.chromium.org/1154313003/diff/80001/base/test/launcher/test...
base/test/launcher/test_launcher_nacl_nonsfi.h:13: int TestLauncherMain(const
std::string& test_binary);
On 2015/06/02 10:37:36, Paweł Hajdan Jr. wrote:
> Could you include something related to NaCl and Non-SFI in the name of this
> function? Otherwise it seems too generic.

Done.

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/run_all_unittests.cc (right):

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/run_all_unittests.cc:13: argc, argv,
base::Bind(&RUN_ALL_TESTS));
On 2015/06/02 10:37:36, Paweł Hajdan Jr. wrote:
> It seems a little bit dangerous to apply & directly to a macro. Could you wrap
> it in a function?

RUN_ALL_TESTS is a function (though, indeed, it looks macro).

https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

[Paweł Hajdan Jr., 2015-06-02 15:41:38]

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/run_all_unittests.cc (right):

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/run_all_unittests.cc:13: argc, argv,
base::Bind(&RUN_ALL_TESTS));
On 2015/06/02 14:55:20, hidehiko wrote:
> On 2015/06/02 10:37:36, Paweł Hajdan Jr. wrote:
> > It seems a little bit dangerous to apply & directly to a macro. Could you
wrap
> > it in a function?
> 
> RUN_ALL_TESTS is a function (though, indeed, it looks macro).
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

Oh, indeed. Still, it looks like a macro and was a macro in the past, so I'd
prefer it being wrapped just to serve as a good example.

https://codereview.chromium.org/1154313003/diff/120001/base/test/launcher/tes...
File base/test/launcher/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/120001/base/test/launcher/tes...
base/test/launcher/test_launcher_nacl_nonsfi.cc:98: if
(!subprocess.WaitForExit(nullptr)) {
Why not use LaunchOptions.wait = true instead?

It looks equivalent and sort of a nit, but I think there's value in doing same
thing in consistent way within the test launcher.

https://codereview.chromium.org/1154313003/diff/120001/base/test/launcher/tes...
base/test/launcher/test_launcher_nacl_nonsfi.cc:99: LOG(ERROR) << "Failed to run
test with --"
nit: No need for this log statement, please remove for consistency (same as
above).

[hidehiko, 2015-06-02 18:34:01]

Thank you for comment. PTAL.

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/run_all_unittests.cc (right):

https://codereview.chromium.org/1154313003/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/run_all_unittests.cc:13: argc, argv,
base::Bind(&RUN_ALL_TESTS));
On 2015/06/02 15:41:38, Paweł Hajdan Jr. wrote:
> On 2015/06/02 14:55:20, hidehiko wrote:
> > On 2015/06/02 10:37:36, Paweł Hajdan Jr. wrote:
> > > It seems a little bit dangerous to apply & directly to a macro. Could you
> wrap
> > > it in a function?
> > 
> > RUN_ALL_TESTS is a function (though, indeed, it looks macro).
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
> 
> Oh, indeed. Still, it looks like a macro and was a macro in the past, so I'd
> prefer it being wrapped just to serve as a good example.

Hmm, ok. Done.

https://codereview.chromium.org/1154313003/diff/120001/base/test/launcher/tes...
File base/test/launcher/test_launcher_nacl_nonsfi.cc (right):

https://codereview.chromium.org/1154313003/diff/120001/base/test/launcher/tes...
base/test/launcher/test_launcher_nacl_nonsfi.cc:98: if
(!subprocess.WaitForExit(nullptr)) {
On 2015/06/02 15:41:38, Paweł Hajdan Jr. wrote:
> Why not use LaunchOptions.wait = true instead?
> 
> It looks equivalent and sort of a nit, but I think there's value in doing same
> thing in consistent way within the test launcher.

Oh, I misunderstood your intention. I referred test_launcher_ios.cc, now. Done.

https://codereview.chromium.org/1154313003/diff/120001/base/test/launcher/tes...
base/test/launcher/test_launcher_nacl_nonsfi.cc:99: LOG(ERROR) << "Failed to run
test with --"
On 2015/06/02 15:41:38, Paweł Hajdan Jr. wrote:
> nit: No need for this log statement, please remove for consistency (same as
> above).

Done.

[Paweł Hajdan Jr., 2015-06-03 14:46:39]

LGTM

[hidehiko, 2015-06-03 17:11:19]

On 2015/06/03 14:46:39, Paweł Hajdan Jr. wrote:
> LGTM

Thank you for review, Pawel.

Nico, Mark, ping?
- hidehiko

[hidehiko, 2015-06-04 14:47:32]

On 2015/06/03 17:11:19, hidehiko wrote:
> On 2015/06/03 14:46:39, Paweł Hajdan Jr. wrote:
> > LGTM
> 
> Thank you for review, Pawel.
> 
> Nico, Mark, ping?
> - hidehiko

Nico, Mark, ping?

[Nico, 2015-06-04 18:13:41]

build/all.gyp lgtm

https://codereview.chromium.org/1154313003/diff/140001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1154313003/diff/140001/base/base.gyp#newcode1063
base/base.gyp:1063: ['disable_nacl==0 and disable_nacl_untrusted==0 and
enable_nacl_nonsfi_test==1', {
nit: the preferred pattern is to define the target unconditionally and
conditionally give it either the real type or type 'none' like so:

  {
    'target_name': '...'
    'conditions': ['disable_nacl==0 and...;', {
      'type': 'static_library',
      'sources': ...
      ...
    }, {
      'type': 'none',
    }],
  }

That way, if something depends on it, the dependency can be unconditional

[hidehiko, 2015-06-04 19:38:09]

Thank you for review. PTAL for base/..., Nico.

Mark, ping?

- hidehiko

https://codereview.chromium.org/1154313003/diff/140001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1154313003/diff/140001/base/base.gyp#newcode1063
base/base.gyp:1063: ['disable_nacl==0 and disable_nacl_untrusted==0 and
enable_nacl_nonsfi_test==1', {
On 2015/06/04 18:13:41, Nico wrote:
> nit: the preferred pattern is to define the target unconditionally and
> conditionally give it either the real type or type 'none' like so:
> 
>   {
>     'target_name': '...'
>     'conditions': ['disable_nacl==0 and...;', {
>       'type': 'static_library',
>       'sources': ...
>       ...
>     }, {
>       'type': 'none',
>     }],
>   }
> 
> That way, if something depends on it, the dependency can be unconditional

Sure, done.

Note: I kept remaining rules as is in this CL, to keep the consistency in a
file.

[Mark Seaborn, 2015-06-04 21:43:56]

LGTM.  Mostly a rubber-stamp because I'm not familiar with the gtest plumbing,
and I assume Pawel reviewed that.

https://codereview.chromium.org/1154313003/diff/180001/base/test/gtest_xml_un...
File base/test/gtest_xml_unittest_result_printer.h (right):

https://codereview.chromium.org/1154313003/diff/180001/base/test/gtest_xml_un...
base/test/gtest_xml_unittest_result_printer.h:20: class XmlUnitTestResultPrinter
: public testing::EmptyTestEventListener {
In the commit message, as a summary, it would be helpful to say explicitly that
you're splitting this out into a separate file.

https://codereview.chromium.org/1154313003/diff/180001/base/test/launcher/tes...
File base/test/launcher/test_launcher_nacl_nonsfi.h (right):

https://codereview.chromium.org/1154313003/diff/180001/base/test/launcher/tes...
base/test/launcher/test_launcher_nacl_nonsfi.h:15: }  // namespcae base
"namespace"

[hidehiko, 2015-06-08 17:14:39]

Thank you for review, Mark.

Nico, friendly ping for base/... ?

Thanks,
- hidehiko

https://codereview.chromium.org/1154313003/diff/180001/base/test/gtest_xml_un...
File base/test/gtest_xml_unittest_result_printer.h (right):

https://codereview.chromium.org/1154313003/diff/180001/base/test/gtest_xml_un...
base/test/gtest_xml_unittest_result_printer.h:20: class XmlUnitTestResultPrinter
: public testing::EmptyTestEventListener {
On 2015/06/04 21:43:55, Mark Seaborn wrote:
> In the commit message, as a summary, it would be helpful to say explicitly
that
> you're splitting this out into a separate file.

Done.

https://codereview.chromium.org/1154313003/diff/180001/base/test/launcher/tes...
File base/test/launcher/test_launcher_nacl_nonsfi.h (right):

https://codereview.chromium.org/1154313003/diff/180001/base/test/launcher/tes...
base/test/launcher/test_launcher_nacl_nonsfi.h:15: }  // namespcae base
On 2015/06/04 21:43:56, Mark Seaborn wrote:
> "namespace"

Done.

[Nico, 2015-06-08 17:16:45]

base lgtm too. sorry, i missed that you wanted me to look at that as well.

[hidehiko, 2015-06-09 01:24:53]

Thank you for review, again, Nico.

Submitting.

[hidehiko, 2015-06-09 01:25:00]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-06-09 01:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1154313003/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-06-09 01:27:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154313003/220001

[commit-bot: I haz the power, 2015-06-09 02:51:05]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-06-09 02:52:15]

Patchset 11 (id:??) landed as
https://crrev.com/8fc7c823b9dd2b7890c96438f347383aa713c1d6
Cr-Commit-Position: refs/heads/master@{#333430}