Use is_valid_dump to check for valid dumps in telemetry exceptions.

Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
TBR=jochen@chromium.org
BUG=561763
Committed: https://crrev.com/c6be24867adfaa4159909e2072778fbe3fc341fd
Cr-Commit-Position: refs/heads/master@{#387386}

[David Yen, 2016-04-13 16:35:04]

On 2016/04/12 23:01:24, David Yen wrote:

Hmm... it looks like some tests are not working, they were just being covered up
by the wrong indentation. I will have to investigate.

[David Yen, 2016-04-13 18:37:07]

On 2016/04/13 16:35:04, David Yen wrote:
> On 2016/04/12 23:01:24, David Yen wrote:
> 
> Hmm... it looks like some tests are not working, they were just being covered
up
> by the wrong indentation. I will have to investigate.

In the 1 week we had broken the stacktrace_unittest all 3 platforms are
failing...

[David Yen, 2016-04-13 22:59:59]

dyen@chromium.org changed reviewers:
+ jochen@chromium.org

[David Yen, 2016-04-13 22:59:59]

+jochen@ for generate_breakpad_symbols.py review, I found a bug in how I'm
splitting the header. Split takes in the maximum number of splits, not the
maximum number of results. So to get a maximum result of 5, you need to pass it
4 splits.

[David Yen, 2016-04-14 00:18:39]

It looks like the windows mojo crash has been fixed, so I'm re-enabling it.
PTAL. I also improved the bad breakpad file test so that the garbage file
actually looks like a valid symbol file.

[nednguyen, 2016-04-14 00:35:41]

tools/perf lgtm

https://codereview.chromium.org/1880333002/diff/100001/tools/perf/core/stackt...
File tools/perf/core/stacktrace_unittest.py (left):

https://codereview.chromium.org/1880333002/diff/100001/tools/perf/core/stackt...
tools/perf/core/stacktrace_unittest.py:60: # A single symbol file should still
exist here.
This logic is no longer important to test?

[David Yen, 2016-04-14 00:38:13]

https://codereview.chromium.org/1880333002/diff/100001/tools/perf/core/stackt...
File tools/perf/core/stacktrace_unittest.py (left):

https://codereview.chromium.org/1880333002/diff/100001/tools/perf/core/stackt...
tools/perf/core/stacktrace_unittest.py:60: # A single symbol file should still
exist here.
On 2016/04/14 00:35:41, nednguyen wrote:
> This logic is no longer important to test?

This doesn't work on Mac because the binary with symbols is chromium framework
and not the original executable. This test basically never worked but the
asserts were not firing

[jochen (gone - plz use gerrit), 2016-04-14 13:30:09]

https://codereview.chromium.org/1880333002/diff/100001/components/crash/conte...
File components/crash/content/tools/generate_breakpad_symbols.py (right):

https://codereview.chromium.org/1880333002/diff/100001/components/crash/conte...
components/crash/content/tools/generate_breakpad_symbols.py:148: # header info
is of the form "MODULE $PLATFORM $ARCH $HASH $BINARY"
is the comment still up to date? Can you explain why it would only split it in
four?

https://codereview.chromium.org/1880333002/diff/100001/components/crash/conte...
components/crash/content/tools/generate_breakpad_symbols.py:150: if
len(info_split) != 5 or info_split[0] != 'MODULE':
the first condition is now basically always true, no?

[David Yen, 2016-04-14 16:46:06]

https://codereview.chromium.org/1880333002/diff/100001/components/crash/conte...
File components/crash/content/tools/generate_breakpad_symbols.py (right):

https://codereview.chromium.org/1880333002/diff/100001/components/crash/conte...
components/crash/content/tools/generate_breakpad_symbols.py:148: # header info
is of the form "MODULE $PLATFORM $ARCH $HASH $BINARY"
On 2016/04/14 13:30:09, jochen - slow wrote:
> is the comment still up to date? Can you explain why it would only split it in
> four?

I was confused about this too, the max splits is 1 less than the max number of
results: so "1 2" with with 1 split is [1], [2], "1 2 3 4 5" with 4 splits is
[1], [2], [3], [4], [5]. In other words, it's the number of commas.

https://codereview.chromium.org/1880333002/diff/100001/components/crash/conte...
components/crash/content/tools/generate_breakpad_symbols.py:150: if
len(info_split) != 5 or info_split[0] != 'MODULE':
On 2016/04/14 13:30:09, jochen - slow wrote:
> the first condition is now basically always true, no?

see explanation above

[David Yen, 2016-04-14 17:11:15]

Ugh, I had to disable windows again. It is literally regressing between try
jobs.

[David Yen, 2016-04-14 18:59:58]

Description was changed from

==========
Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
BUG=561763
==========

to

==========
Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
TBR=jochen@chromium.org
BUG=561763
==========

[David Yen, 2016-04-14 19:00:41]

I will TBR jochen@ since it's a pretty trivial change and this test is allowing
regressions to land at a very rapid pace.

[David Yen, 2016-04-14 19:01:20]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-04-14 19:01:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1880333002/#ps140001
(title: "Disable windows for testValidDump")

[commit-bot: I haz the power, 2016-04-14 19:01:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880333002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880333002/140001

[commit-bot: I haz the power, 2016-04-14 19:12:16]

Description was changed from

==========
Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
TBR=jochen@chromium.org
BUG=561763
==========

to

==========
Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
TBR=jochen@chromium.org
BUG=561763
==========

[commit-bot: I haz the power, 2016-04-14 19:12:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-14 19:12:48]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-04-14 19:13:52]

Description was changed from

==========
Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
TBR=jochen@chromium.org
BUG=561763
==========

to

==========
Use is_valid_dump to check for valid dumps in telemetry exceptions.

Now that we have a property which stores whether or not a stack trace
is valid, we can use that to test for valid minidumps instead of looking
for platform dependent strings. This test should also work in Android
now so we can enable it on all platforms except for chromeos.

This CL also fixes an issue where asserts raised within an assertRaises
block would be eaten, so tests were not failing as expected.

R=nednguyen@google.com
TBR=jochen@chromium.org
BUG=561763

Committed: https://crrev.com/c6be24867adfaa4159909e2072778fbe3fc341fd
Cr-Commit-Position: refs/heads/master@{#387386}
==========

[commit-bot: I haz the power, 2016-04-14 19:13:53]

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