Handle and surface application test failures.

Handle and surface application test failures.

Handle failures while listing or executing apptests.
(check the exit codes and output for both)
Continue other apptests, but bail on other fixtures.

BUG=392646
TEST=NONE
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/c6fc9551ea416bd21f9e5ffea7bda2023b849b7b

[msw, 2014-11-21 20:30:55]

msw@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[msw, 2014-11-21 20:31:25]

Hey Trung, please take a look; thanks!

[viettrungluu, 2014-11-21 22:11:38]

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:12: import string
Why do you need this?

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:19: 
one more blank line, apparently

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:41: if not list_output:
You should probably let gtest_list_tests decide about the validity of the output
(possibly raising an exception if necessary).

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:44: if not re.match("^(\w*\.\n(  \w*\n)+)+",
list_output):
... thus you wouldn't need this.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:57: if re.search("\[  FAILED  \]", output):
You could just do something like output.find("[  FAILED  ]").

Then you wouldn't need re at all.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:63: print "Failed with exit code %d and output:" %
e.returncode
Hmmm, I don't like the code duplication much.

Consider factoring out this code (to return output on success, None on failure).

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:109: # Skip remaining fixtures on failure, but run
remaining apptest suites.
We're probably going to move away from this, so you may as well not do this (the
break, that is).

[msw, 2014-11-22 01:07:38]

Please take another look; thanks!

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:12: import string
On 2014/11/21 22:11:38, viettrungluu wrote:
> Why do you need this?

Removed (needed this in an older iteration of the code).

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:19: 
On 2014/11/21 22:11:38, viettrungluu wrote:
> one more blank line, apparently

Done.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:41: if not list_output:
On 2014/11/21 22:11:38, viettrungluu wrote:
> You should probably let gtest_list_tests decide about the validity of the
output
> (possibly raising an exception if necessary).

Done.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:44: if not re.match("^(\w*\.\n(  \w*\n)+)+",
list_output):
On 2014/11/21 22:11:38, viettrungluu wrote:
> ... thus you wouldn't need this.

Done.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:57: if re.search("\[  FAILED  \]", output):
On 2014/11/21 22:11:38, viettrungluu wrote:
> You could just do something like output.find("[  FAILED  ]").
> 
> Then you wouldn't need re at all.

Done.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:63: print "Failed with exit code %d and output:" %
e.returncode
On 2014/11/21 22:11:38, viettrungluu wrote:
> Hmmm, I don't like the code duplication much.
> 
> Consider factoring out this code (to return output on success, None on
failure).

Done. The result may bit a little more verbose on failure.

https://codereview.chromium.org/741323002/diff/40001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:109: # Skip remaining fixtures on failure, but run
remaining apptest suites.
On 2014/11/21 22:11:38, viettrungluu wrote:
> We're probably going to move away from this, so you may as well not do this
(the
> break, that is).

Done.

[msw, 2014-11-22 01:10:31]

Patchset #4 (id:60001) has been deleted

[viettrungluu, 2014-11-24 20:22:58]

lgtm w/nits

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:50: return []
I'd indent this one more level, since it's only used in the except ...: case.

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:63: else:
nit: No else needed.

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/mopy/gtest_li...
File mojo/tools/mopy/gtest_list_tests.py (right):

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/mopy/gtest_li...
mojo/tools/mopy/gtest_list_tests.py:13: raise Exception ("Unrecognized
--gtest_list_tests output:\n%s" %
nit: No space before '('.

[msw, 2014-11-24 20:46:54]

Comments addressed, I also synced and rebased for:
<https://codereview.chromium.org/744973002/> and tweaked the command lines to
use --args-for=... instead of passing args within the application argument to
the shell. Landing.

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/apptest_runne...
File mojo/tools/apptest_runner.py (right):

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:50: return []
On 2014/11/24 20:22:58, viettrungluu wrote:
> I'd indent this one more level, since it's only used in the except ...: case.

Done.

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/apptest_runne...
mojo/tools/apptest_runner.py:63: else:
On 2014/11/24 20:22:57, viettrungluu wrote:
> nit: No else needed.

Done.

[msw, 2014-11-24 20:47:31]

[missed responding to one comment, but it was addressed]

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/mopy/gtest_li...
File mojo/tools/mopy/gtest_list_tests.py (right):

https://codereview.chromium.org/741323002/diff/80001/mojo/tools/mopy/gtest_li...
mojo/tools/mopy/gtest_list_tests.py:13: raise Exception ("Unrecognized
--gtest_list_tests output:\n%s" %
On 2014/11/24 20:22:58, viettrungluu wrote:
> nit: No space before '('.

Done.

[msw, 2014-11-24 20:55:09]

Committed patchset #6 (id:120001) manually as
c6fc9551ea416bd21f9e5ffea7bda2023b849b7b (presubmit successful).