Add an end-to-end test for symbolize_trace on macOS.

Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
    symbols.
  * Non-system symbols are symbolized as <library> + offset
  * If a system library cannot be found, symbolize as <library> + offset rather
    than <library>
  * Add test data for macOS symbolization [pre and post symbolization]
  * This new tests is slow (~1 minute on a fast macOS machine on the second run,
  >5 minutes on first run) so it's not added to the run_py_tests suite. Instead,
  I added a new test suite run_symbolizer_tests that can be invoked to run this
  test suite. run_symbolizer_tests is run as part of the Catapult
  continuous-integration test suite on the waterfall.


BUG=chromium:729103, chromium:734852
Review-Url: https://codereview.chromium.org/2950723002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/28f796f29c9e269f992fbb1550b0e384a227f602

[erikchen, 2017-06-20 01:28:58]

erikchen@chromium.org changed reviewers:
+ dskiba@chromium.org, etienneb@chromium.org

[erikchen, 2017-06-20 01:28:58]

etienneb, dskiba: PTAL

[etienneb, 2017-06-20 03:28:09]

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/run_symbolize_t...
File tracing/bin/run_symbolize_trace_tests (right):

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/run_symbolize_t...
tracing/bin/run_symbolize_trace_tests:21: args = [
'download_from_google_storage.py', '-s', path + '.sha1', '--bucket', BUCKET ]
split line

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/run_symbolize_t...
tracing/bin/run_symbolize_trace_tests:43: success = (GetGzipCrc(temporary_trace)
why comparing CRC?
File comparison should be the same cost.

This way we can avoid the shell commands, and I may be able to port this test to
windows.

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace.py
File tracing/bin/symbolize_trace.py (right):

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace...
tracing/bin/symbolize_trace.py:1072: unsymbolized_name = ('<' +
symfile.symbolizable_path + '> + ' +
I wonder if we should not use only basename instead of the full path

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace...
tracing/bin/symbolize_trace.py:1117: str(address))
nit: address should be hexa!?

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace...
tracing/bin/symbolize_trace.py:1379: parser.add_argument(
For not chromium trace (traces from user computers) we should put that flag "on"
by default.

Or

We could symbolize address + offset by default, and put a flag to also symbolize
system libraries.

[erikchen, 2017-06-23 00:30:59]

Description was changed from

==========
Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * rename symbolize_trace to symbolize_trace.py to allow python import.
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
  symbols.
  * Add test data for macOS symbolization [pre and post symbolization]
  * Slight refactor to allow calling symbolize_trace.main() with explicit
  arguments.

This CL runs slowly, so it's not integration into the waterfall tests and must
be run manually.


BUG=chromium:729103, chromium:734852
==========

to

==========
Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
  symbols.
  * Add test data for macOS symbolization [pre and post symbolization]
  * This new tests is slow (~1 minute on a fast macOS machine on the second run,
  >5 minutes on first run) so it's not added to the run_py_tests suite. Instead,
  I added a new test suite run_symbolizer_tests that can be invoked to run this
  test suite. run_symbolizer_tests is run as part of the Catapult
  continuous-integration test suite on the waterfall.


BUG=chromium:729103, chromium:734852
==========

[erikchen, 2017-06-23 00:31:11]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 00:31:15]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-06-23 00:33:45]

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/run_symbolize_t...
File tracing/bin/run_symbolize_trace_tests (right):

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/run_symbolize_t...
tracing/bin/run_symbolize_trace_tests:21: args = [
'download_from_google_storage.py', '-s', path + '.sha1', '--bucket', BUCKET ]
On 2017/06/20 03:28:09, etienneb wrote:
> split line

Done.

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/run_symbolize_t...
tracing/bin/run_symbolize_trace_tests:43: success = (GetGzipCrc(temporary_trace)
On 2017/06/20 03:28:09, etienneb wrote:
> why comparing CRC?
> File comparison should be the same cost.
> 
> This way we can avoid the shell commands, and I may be able to port this test
to
> windows.

gzip files have a timestamp built in, so can't be directly compared.

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace.py
File tracing/bin/symbolize_trace.py (right):

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace...
tracing/bin/symbolize_trace.py:1072: unsymbolized_name = ('<' +
symfile.symbolizable_path + '> + ' +
On 2017/06/20 03:28:09, etienneb wrote:
> I wonder if we should not use only basename instead of the full path

Done.

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace...
tracing/bin/symbolize_trace.py:1117: str(address))
On 2017/06/20 03:28:09, etienneb wrote:
> nit: address should be hexa!?

Done.

https://codereview.chromium.org/2950723002/diff/1/tracing/bin/symbolize_trace...
tracing/bin/symbolize_trace.py:1379: parser.add_argument(
On 2017/06/20 03:28:09, etienneb wrote:
> For not chromium trace (traces from user computers) we should put that flag
"on"
> by default.
> 
> Or
> 
> We could symbolize address + offset by default, and put a flag to also
symbolize
> system libraries.

Done.

[erikchen, 2017-06-23 00:34:04]

erikchen@chromium.org changed reviewers:
+ eakuefner@chromium.org

[erikchen, 2017-06-23 00:34:05]

eakuefner: Please review.

[commit-bot: I haz the power, 2017-06-23 00:54:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 00:54:06]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[erikchen, 2017-06-23 16:53:34]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 16:53:41]

Dry run: 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, 2017-06-23 16:58:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 16:58:52]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[erikchen, 2017-06-23 18:08:55]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 18:09:01]

Dry run: 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, 2017-06-23 18:14:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 18:14:12]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[erikchen, 2017-06-23 18:18:10]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 18:18:22]

Dry run: 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, 2017-06-23 18:23:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 18:23:32]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[erikchen, 2017-06-23 21:59:59]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 22:00:09]

Dry run: CQ is trying da patch.

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

[eakuefner, 2017-06-23 22:05:26]

Thanks Erik! lgtm with nits.

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py
(right):

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:39:
def testTestMacv1(self):
Why not just testMacV1?

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:44:
_THIS_DIR_PATH, 'data', 'mac_trace_v1_presymbolization.json.gz')
nit: You only ever suffix _THIS_DIR_PATH with 'data', so why not move 'data'
into the constant and call it _DATA_PATH?

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:58:
except:
Probably you don't actually want this; eliding the except block will cause the
exception to automatically be re-raised following the execution of the finally.

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:61:
os.remove(temporary_trace)
This itself will raise OSError if temporary_trace has not been created, so
probably you want to guard it with

if os.path.exists(temporary_trace):

[eakuefner, 2017-06-23 22:06:11]

eakuefner@chromium.org changed reviewers:
+ nednguyen@google.com

[eakuefner, 2017-06-23 22:06:12]

+nednguyen for catapult_build OWNERS

[commit-bot: I haz the power, 2017-06-23 22:12:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 22:12:04]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[erikchen, 2017-06-23 23:18:22]

Description was changed from

==========
Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
  symbols.
  * Add test data for macOS symbolization [pre and post symbolization]
  * This new tests is slow (~1 minute on a fast macOS machine on the second run,
  >5 minutes on first run) so it's not added to the run_py_tests suite. Instead,
  I added a new test suite run_symbolizer_tests that can be invoked to run this
  test suite. run_symbolizer_tests is run as part of the Catapult
  continuous-integration test suite on the waterfall.


BUG=chromium:729103, chromium:734852
==========

to

==========
Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
    symbols.
  * Non-system symbols are symbolized as <library> + offset
  * If a system library cannot be found, symbolize as <library> + offset rather
    than <library>
  * Add test data for macOS symbolization [pre and post symbolization]
  * This new tests is slow (~1 minute on a fast macOS machine on the second run,
  >5 minutes on first run) so it's not added to the run_py_tests suite. Instead,
  I added a new test suite run_symbolizer_tests that can be invoked to run this
  test suite. run_symbolizer_tests is run as part of the Catapult
  continuous-integration test suite on the waterfall.


BUG=chromium:729103, chromium:734852
==========

[nednguyen, 2017-06-24 00:18:40]

lgtm

[erikchen, 2017-06-24 00:20:19]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 00:20:24]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-06-24 00:20:57]

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py
(right):

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:39:
def testTestMacv1(self):
On 2017/06/23 22:05:26, eakuefner wrote:
> Why not just testMacV1?

Done.

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:44:
_THIS_DIR_PATH, 'data', 'mac_trace_v1_presymbolization.json.gz')
On 2017/06/23 22:05:26, eakuefner wrote:
> nit: You only ever suffix _THIS_DIR_PATH with 'data', so why not move 'data'
> into the constant and call it _DATA_PATH?

Used on line 16 with data

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:58:
except:
On 2017/06/23 22:05:26, eakuefner wrote:
> Probably you don't actually want this; eliding the except block will cause the
> exception to automatically be re-raised following the execution of the
finally.

Done.

https://codereview.chromium.org/2950723002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/symbolizer/symbolize_trace_end_to_end_test_slow.py:61:
os.remove(temporary_trace)
On 2017/06/23 22:05:26, eakuefner wrote:
> This itself will raise OSError if temporary_trace has not been created, so
> probably you want to guard it with
> 
> if os.path.exists(temporary_trace):

Done.

[erikchen, 2017-06-24 00:44:00]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-06-24 00:44:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2950723002/#ps140001
(title: "lint")

[erikchen, 2017-06-24 00:44:02]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-06-24 00:44:08]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-06-24 00:44:19]

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, 2017-06-24 00:45:52]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498265048563780,
"parent_rev": "677b02eacd6fe0d32040649858b5248d7dc402da", "commit_rev":
"28f796f29c9e269f992fbb1550b0e384a227f602"}

[commit-bot: I haz the power, 2017-06-24 00:45:59]

Description was changed from

==========
Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
    symbols.
  * Non-system symbols are symbolized as <library> + offset
  * If a system library cannot be found, symbolize as <library> + offset rather
    than <library>
  * Add test data for macOS symbolization [pre and post symbolization]
  * This new tests is slow (~1 minute on a fast macOS machine on the second run,
  >5 minutes on first run) so it's not added to the run_py_tests suite. Instead,
  I added a new test suite run_symbolizer_tests that can be invoked to run this
  test suite. run_symbolizer_tests is run as part of the Catapult
  continuous-integration test suite on the waterfall.


BUG=chromium:729103, chromium:734852
==========

to

==========
Add an end-to-end test for symbolize_trace on macOS.

Changes included in this CL:
  * Add a only-symbolize-chrome-symbols mode to prevent symbolization of system
    symbols.
  * Non-system symbols are symbolized as <library> + offset
  * If a system library cannot be found, symbolize as <library> + offset rather
    than <library>
  * Add test data for macOS symbolization [pre and post symbolization]
  * This new tests is slow (~1 minute on a fast macOS machine on the second run,
  >5 minutes on first run) so it's not added to the run_py_tests suite. Instead,
  I added a new test suite run_symbolizer_tests that can be invoked to run this
  test suite. run_symbolizer_tests is run as part of the Catapult
  continuous-integration test suite on the waterfall.


BUG=chromium:729103, chromium:734852

Review-Url: https://codereview.chromium.org/2950723002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-06-24 00:46:00]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...