Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1498633003:
Added unit test for stack traces in telemetry crashes. (Closed)
DescriptionAdded unit test for stack traces in telemetry crashes.
This CL also fixes an issue on Mac where telemetry does not
know how to locate chromium_framework for the symbols.
R=nednguyen@google.com
BUG=563716
TEST=ran unit test locally
Committed: https://crrev.com/a20e03ee6906c50bdfaf8fb34fe684b4a89db852
Cr-Commit-Position: refs/heads/master@{#368997}
Patch Set 1 #Patch Set 2 : Moved test to tools/perf/core, made stack trace a property of AppCrashException #Patch Set 3 : Changed exception to DevtoolsTargetCrashException #Patch Set 4 : bad upload? #Patch Set 5 : stack trace is in a list form #Patch Set 6 : Search for framework file #
Total comments: 6
Patch Set 7 : Changed to use assertIn and run isolated. #Patch Set 8 : upload crashdump #Patch Set 9 : Add logging for crash reports found #Patch Set 10 : Modify symbol directory to include sha1 version of the name #Patch Set 11 : Use relative symbolic link instead #Patch Set 12 : Test android instead #Patch Set 13 : Add different unit test to just test stack traces are present. #Patch Set 14 : Use Disabled(all) #Patch Set 15 : Removed debug code #Patch Set 16 : Revert unnecessary changes #Patch Set 17 : Just enable for mac for now, focus on one platform #
Messages
Total messages: 37 (12 generated)
Description was changed from ========== Added unit test for proper stack traces in telemetry crashes. R=nednguyen@chromium.org BUG=563716 ========== to ========== Added unit test for proper stack traces in telemetry crashes. R=nednguyen@google.com BUG=563716 ==========
dyen@chromium.org changed reviewers: + nednguyen@google.com - nednguyen@chromium.org
I know in the other CL you told me to add this into tools/perf but it doesn't seem to fit there. It seems to me that this is still the proper place because stack traces are handled in tools/telemetry/telemetry/core. Are these going to be moved to a different repository separately from perf? I added the unit test back into tools/telemetry for now, let me know where you think it should go.
On 2015/12/02 23:59:35, David Yen wrote: > I know in the other CL you told me to add this into tools/perf but it doesn't > seem to fit there. It seems to me that this is still the proper place because > stack traces are handled in tools/telemetry/telemetry/core. Are these going to > be moved to a different repository separately from perf? > > I added the unit test back into tools/telemetry for now, let me know where you > think it should go. No, this test should be moved to tools/perf because of the tools used for symbolizing the stack will not be packaged with telemetry once it's moved to a standalone project in catapult.
git cl upload is giving me errors when uploading but it looks like it is showing up here, let's hope that worked... PTAL.
On 2015/12/03 00:37:49, David Yen wrote: > git cl upload is giving me errors when uploading but it looks like it is showing > up here, let's hope that worked... PTAL. I will wait for the try bots to be green to review this.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1498633003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1498633003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/12/03 05:21:32, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) core.stacktrace_unittest.TabStackTraceTest.testCrashSymbols is failing, which means we haven't fixed the problem yet :-(
Description was changed from ========== Added unit test for proper stack traces in telemetry crashes. R=nednguyen@google.com BUG=563716 ========== to ========== Added unit test for proper stack traces in telemetry crashes. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ==========
On 2015/12/03 16:44:15, nednguyen wrote: > On 2015/12/03 05:21:32, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > core.stacktrace_unittest.TabStackTraceTest.testCrashSymbols is failing, which > means we haven't fixed the problem yet :-( I ran the unit test locally and it works now, PTAL.
Looks like the test is still failing https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... File tools/perf/core/stacktrace_unittest.py (right): https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... tools/perf/core/stacktrace_unittest.py:18: self.assertTrue('CrashIntentionally' in '\n'.join(e.stack_trace)) self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when this fails, the exception message will tell you what is in the 2nd argument.
I download the isolated to my mac & run the test and it also passes. So my suspicious is the test failing on bot due to many other browser tests that run in parallel that may mess up the out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium Framework.framework/Chromium Framework file https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... File tools/perf/core/stacktrace_unittest.py (right): https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') Add: @decorators.Isolated so that this test is not run in parallel with other telemetry test that also invokes browser. https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... tools/perf/core/stacktrace_unittest.py:15: try: Maybe you need to clear out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium Framework.framework/Chromium Framework directory first?
Multiple crashes at once is most likely the issue here, there could be multiple crash dumps so the stack trace just prints out the latest one. Perhaps we should generate a unique crash dump directory that gets passed in to the browser for each run? That way we can locate the right crash dump. Actually now that I think about it, this is probably why the test was flaky before and why it got reverted. https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... File tools/perf/core/stacktrace_unittest.py (right): https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') On 2015/12/17 02:26:54, nednguyen wrote: > Add: @decorators.Isolated so that this test is not run in parallel with other > telemetry test that also invokes browser. Hmm, I've added this now but that's a little bit worrisome because it means stack traces will be racey and might not show the right stack. https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... tools/perf/core/stacktrace_unittest.py:15: try: On 2015/12/17 02:26:54, nednguyen wrote: > Maybe you need to clear > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > Framework.framework/Chromium Framework directory first? That file is part of the executable so that shouldn't be the racey part. I suspect it is probably the crash dumps themselves. I believe last time I looked into this it just looked in the crash dump directory and looked for the latest one. https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... tools/perf/core/stacktrace_unittest.py:18: self.assertTrue('CrashIntentionally' in '\n'.join(e.stack_trace)) On 2015/12/17 01:52:45, nednguyen wrote: > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when this > fails, the exception message will tell you what is in the 2nd argument. Done.
On 2015/12/21 17:47:50, David Yen wrote: > Multiple crashes at once is most likely the issue here, there could be multiple > crash dumps so the stack trace just prints out the latest one. > > Perhaps we should generate a unique crash dump directory that gets passed in to > the browser for each run? That way we can locate the right crash dump. Hmhh, seems like telemetry is already doing that? https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Actually now that I think about it, this is probably why the test was flaky > before and why it got reverted. > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > File tools/perf/core/stacktrace_unittest.py (right): > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') > On 2015/12/17 02:26:54, nednguyen wrote: > > Add: @decorators.Isolated so that this test is not run in parallel with other > > telemetry test that also invokes browser. > > Hmm, I've added this now but that's a little bit worrisome because it means > stack traces will be racey and might not show the right stack. > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > tools/perf/core/stacktrace_unittest.py:15: try: > On 2015/12/17 02:26:54, nednguyen wrote: > > Maybe you need to clear > > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > > Framework.framework/Chromium Framework directory first? > > That file is part of the executable so that shouldn't be the racey part. I > suspect it is probably the crash dumps themselves. I believe last time I looked > into this it just looked in the crash dump directory and looked for the latest > one. > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > tools/perf/core/stacktrace_unittest.py:18: self.assertTrue('CrashIntentionally' > in '\n'.join(e.stack_trace)) > On 2015/12/17 01:52:45, nednguyen wrote: > > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when this > > fails, the exception message will tell you what is in the 2nd argument. > > Done.
On 2015/12/21 17:59:39, nednguyen wrote: > On 2015/12/21 17:47:50, David Yen wrote: > > Multiple crashes at once is most likely the issue here, there could be > multiple > > crash dumps so the stack trace just prints out the latest one. > > > > Perhaps we should generate a unique crash dump directory that gets passed in > to > > the browser for each run? That way we can locate the right crash dump. > > Hmhh, seems like telemetry is already doing that? > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > Actually now that I think about it, this is probably why the test was flaky > > before and why it got reverted. > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > File tools/perf/core/stacktrace_unittest.py (right): > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') > > On 2015/12/17 02:26:54, nednguyen wrote: > > > Add: @decorators.Isolated so that this test is not run in parallel with > other > > > telemetry test that also invokes browser. > > > > Hmm, I've added this now but that's a little bit worrisome because it means > > stack traces will be racey and might not show the right stack. > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > tools/perf/core/stacktrace_unittest.py:15: try: > > On 2015/12/17 02:26:54, nednguyen wrote: > > > Maybe you need to clear > > > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > > > Framework.framework/Chromium Framework directory first? > > > > That file is part of the executable so that shouldn't be the racey part. I > > suspect it is probably the crash dumps themselves. I believe last time I > looked > > into this it just looked in the crash dump directory and looked for the latest > > one. > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > tools/perf/core/stacktrace_unittest.py:18: > self.assertTrue('CrashIntentionally' > > in '\n'.join(e.stack_trace)) > > On 2015/12/17 01:52:45, nednguyen wrote: > > > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when this > > > fails, the exception message will tell you what is in the 2nd argument. > > > > Done. The test is still failing, the stack it actually output is: http://pastebin.com/Mm4NjpR6 Look like the symbolization fail?
On 2015/12/21 18:24:07, nednguyen wrote: > On 2015/12/21 17:59:39, nednguyen wrote: > > On 2015/12/21 17:47:50, David Yen wrote: > > > Multiple crashes at once is most likely the issue here, there could be > > multiple > > > crash dumps so the stack trace just prints out the latest one. > > > > > > Perhaps we should generate a unique crash dump directory that gets passed in > > to > > > the browser for each run? That way we can locate the right crash dump. > > > > Hmhh, seems like telemetry is already doing that? > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > > Actually now that I think about it, this is probably why the test was flaky > > > before and why it got reverted. > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > File tools/perf/core/stacktrace_unittest.py (right): > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > Add: @decorators.Isolated so that this test is not run in parallel with > > other > > > > telemetry test that also invokes browser. > > > > > > Hmm, I've added this now but that's a little bit worrisome because it means > > > stack traces will be racey and might not show the right stack. > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > tools/perf/core/stacktrace_unittest.py:15: try: > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > Maybe you need to clear > > > > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > > > > Framework.framework/Chromium Framework directory first? > > > > > > That file is part of the executable so that shouldn't be the racey part. I > > > suspect it is probably the crash dumps themselves. I believe last time I > > looked > > > into this it just looked in the crash dump directory and looked for the > latest > > > one. > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > tools/perf/core/stacktrace_unittest.py:18: > > self.assertTrue('CrashIntentionally' > > > in '\n'.join(e.stack_trace)) > > > On 2015/12/17 01:52:45, nednguyen wrote: > > > > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when this > > > > fails, the exception message will tell you what is in the 2nd argument. > > > > > > Done. > > The test is still failing, the stack it actually output is: > http://pastebin.com/Mm4NjpR6 > > Look like the symbolization fail? I added "core.stacktrace_unittest.TabStackTraceTest.testStackTrace" which just tests if the stack trace exists. I verified it's passing in mac_chromium_rel_ng. Lets just land this first. It will also make stack traces work for non-swarming cases. PTAL.
Description was changed from ========== Added unit test for proper stack traces in telemetry crashes. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ========== to ========== Added unit test for stack traces in telemetry crashes. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ==========
Description was changed from ========== Added unit test for stack traces in telemetry crashes. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ========== to ========== Added unit test for stack traces in telemetry crashes. This CL also fixes an issue on Mac where telemetry does not know how to locate chromium_framework for the symbols. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ==========
On 2016/01/12 19:09:01, David Yen wrote: > On 2015/12/21 18:24:07, nednguyen wrote: > > On 2015/12/21 17:59:39, nednguyen wrote: > > > On 2015/12/21 17:47:50, David Yen wrote: > > > > Multiple crashes at once is most likely the issue here, there could be > > > multiple > > > > crash dumps so the stack trace just prints out the latest one. > > > > > > > > Perhaps we should generate a unique crash dump directory that gets passed > in > > > to > > > > the browser for each run? That way we can locate the right crash dump. > > > > > > Hmhh, seems like telemetry is already doing that? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > > > > > Actually now that I think about it, this is probably why the test was > flaky > > > > before and why it got reverted. > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > File tools/perf/core/stacktrace_unittest.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') > > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > > Add: @decorators.Isolated so that this test is not run in parallel with > > > other > > > > > telemetry test that also invokes browser. > > > > > > > > Hmm, I've added this now but that's a little bit worrisome because it > means > > > > stack traces will be racey and might not show the right stack. > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > tools/perf/core/stacktrace_unittest.py:15: try: > > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > > Maybe you need to clear > > > > > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > > > > > Framework.framework/Chromium Framework directory first? > > > > > > > > That file is part of the executable so that shouldn't be the racey part. I > > > > suspect it is probably the crash dumps themselves. I believe last time I > > > looked > > > > into this it just looked in the crash dump directory and looked for the > > latest > > > > one. > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > tools/perf/core/stacktrace_unittest.py:18: > > > self.assertTrue('CrashIntentionally' > > > > in '\n'.join(e.stack_trace)) > > > > On 2015/12/17 01:52:45, nednguyen wrote: > > > > > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when > this > > > > > fails, the exception message will tell you what is in the 2nd argument. > > > > > > > > Done. > > > > The test is still failing, the stack it actually output is: > > http://pastebin.com/Mm4NjpR6 > > > > Look like the symbolization fail? > > I added "core.stacktrace_unittest.TabStackTraceTest.testStackTrace" which just > tests if the stack trace exists. I verified it's passing in mac_chromium_rel_ng. > Lets just land this first. It will also make stack traces work for non-swarming > cases. > > PTAL. lgtm
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1498633003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1498633003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1498633003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1498633003/320001
Message was sent while issue was closed.
Description was changed from ========== Added unit test for stack traces in telemetry crashes. This CL also fixes an issue on Mac where telemetry does not know how to locate chromium_framework for the symbols. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ========== to ========== Added unit test for stack traces in telemetry crashes. This CL also fixes an issue on Mac where telemetry does not know how to locate chromium_framework for the symbols. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Added unit test for stack traces in telemetry crashes. This CL also fixes an issue on Mac where telemetry does not know how to locate chromium_framework for the symbols. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally ========== to ========== Added unit test for stack traces in telemetry crashes. This CL also fixes an issue on Mac where telemetry does not know how to locate chromium_framework for the symbols. R=nednguyen@google.com BUG=563716 TEST=ran unit test locally Committed: https://crrev.com/a20e03ee6906c50bdfaf8fb34fe684b4a89db852 Cr-Commit-Position: refs/heads/master@{#368997} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/a20e03ee6906c50bdfaf8fb34fe684b4a89db852 Cr-Commit-Position: refs/heads/master@{#368997}
Message was sent while issue was closed.
On 2016/01/12 19:09:01, David Yen wrote: > On 2015/12/21 18:24:07, nednguyen wrote: > > On 2015/12/21 17:59:39, nednguyen wrote: > > > On 2015/12/21 17:47:50, David Yen wrote: > > > > Multiple crashes at once is most likely the issue here, there could be > > > multiple > > > > crash dumps so the stack trace just prints out the latest one. > > > > > > > > Perhaps we should generate a unique crash dump directory that gets passed > in > > > to > > > > the browser for each run? That way we can locate the right crash dump. > > > > > > Hmhh, seems like telemetry is already doing that? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > > > > > Actually now that I think about it, this is probably why the test was > flaky > > > > before and why it got reverted. > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > File tools/perf/core/stacktrace_unittest.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') > > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > > Add: @decorators.Isolated so that this test is not run in parallel with > > > other > > > > > telemetry test that also invokes browser. > > > > > > > > Hmm, I've added this now but that's a little bit worrisome because it > means > > > > stack traces will be racey and might not show the right stack. > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > tools/perf/core/stacktrace_unittest.py:15: try: > > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > > Maybe you need to clear > > > > > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > > > > > Framework.framework/Chromium Framework directory first? > > > > > > > > That file is part of the executable so that shouldn't be the racey part. I > > > > suspect it is probably the crash dumps themselves. I believe last time I > > > looked > > > > into this it just looked in the crash dump directory and looked for the > > latest > > > > one. > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > tools/perf/core/stacktrace_unittest.py:18: > > > self.assertTrue('CrashIntentionally' > > > > in '\n'.join(e.stack_trace)) > > > > On 2015/12/17 01:52:45, nednguyen wrote: > > > > > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when > this > > > > > fails, the exception message will tell you what is in the 2nd argument. > > > > > > > > Done. > > > > The test is still failing, the stack it actually output is: > > http://pastebin.com/Mm4NjpR6 > > > > Look like the symbolization fail? > > I added "core.stacktrace_unittest.TabStackTraceTest.testStackTrace" which just > tests if the stack trace exists. I verified it's passing in mac_chromium_rel_ng. > Lets just land this first. It will also make stack traces work for non-swarming > cases. > > PTAL. Excellent work David!
Message was sent while issue was closed.
On 2016/01/13 00:38:19, Ken Russell wrote: > On 2016/01/12 19:09:01, David Yen wrote: > > On 2015/12/21 18:24:07, nednguyen wrote: > > > On 2015/12/21 17:59:39, nednguyen wrote: > > > > On 2015/12/21 17:47:50, David Yen wrote: > > > > > Multiple crashes at once is most likely the issue here, there could be > > > > multiple > > > > > crash dumps so the stack trace just prints out the latest one. > > > > > > > > > > Perhaps we should generate a unique crash dump directory that gets > passed > > in > > > > to > > > > > the browser for each run? That way we can locate the right crash dump. > > > > > > > > Hmhh, seems like telemetry is already doing that? > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > > > > > > > > Actually now that I think about it, this is probably why the test was > > flaky > > > > > before and why it got reverted. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > > File tools/perf/core/stacktrace_unittest.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > > tools/perf/core/stacktrace_unittest.py:13: @decorators.Enabled('mac') > > > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > > > Add: @decorators.Isolated so that this test is not run in parallel > with > > > > other > > > > > > telemetry test that also invokes browser. > > > > > > > > > > Hmm, I've added this now but that's a little bit worrisome because it > > means > > > > > stack traces will be racey and might not show the right stack. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > > tools/perf/core/stacktrace_unittest.py:15: try: > > > > > On 2015/12/17 02:26:54, nednguyen wrote: > > > > > > Maybe you need to clear > > > > > > out/Release/Chromium.app/Contents/Versions/49.0.2594.0/Chromium > > > > > > Framework.framework/Chromium Framework directory first? > > > > > > > > > > That file is part of the executable so that shouldn't be the racey part. > I > > > > > suspect it is probably the crash dumps themselves. I believe last time I > > > > looked > > > > > into this it just looked in the crash dump directory and looked for the > > > latest > > > > > one. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1498633003/diff/100001/tools/perf/core/stackt... > > > > > tools/perf/core/stacktrace_unittest.py:18: > > > > self.assertTrue('CrashIntentionally' > > > > > in '\n'.join(e.stack_trace)) > > > > > On 2015/12/17 01:52:45, nednguyen wrote: > > > > > > self.assertIn('CrashIntentionally', '\n'.join(e.stack_trace)) so when > > this > > > > > > fails, the exception message will tell you what is in the 2nd > argument. > > > > > > > > > > Done. > > > > > > The test is still failing, the stack it actually output is: > > > http://pastebin.com/Mm4NjpR6 > > > > > > Look like the symbolization fail? > > > > I added "core.stacktrace_unittest.TabStackTraceTest.testStackTrace" which just > > tests if the stack trace exists. I verified it's passing in > mac_chromium_rel_ng. > > Lets just land this first. It will also make stack traces work for > non-swarming > > cases. > > > > PTAL. > > Excellent work David! Don't celebrate too early, this fails on some bots. Do these not have the minidump_stackwalk binary being built? https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/ Reverting now.
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/1580313002/ by dyen@chromium.org. The reason for reverting is: Test fails on some bots..
Message was sent while issue was closed.
On 2016/01/13 00:49:54, David Yen wrote: > A revert of this CL (patchset #17 id:320001) has been created in > https://codereview.chromium.org/1580313002/ by mailto:dyen@chromium.org. > > The reason for reverting is: Test fails on some bots.. Which are the Mac bots that this test fails on? We will soon remove the support for Mac below 10.8, so we can just reland this by skipping tests on the failed Mac platform. See: http://chrome.blogspot.com/2015/11/updates-to-chrome-platform-support.html
Message was sent while issue was closed.
On 2016/01/13 14:18:12, nednguyen wrote: > On 2016/01/13 00:49:54, David Yen wrote: > > A revert of this CL (patchset #17 id:320001) has been created in > > https://codereview.chromium.org/1580313002/ by mailto:dyen@chromium.org. > > > > The reason for reverting is: Test fails on some bots.. > > Which are the Mac bots that this test fails on? We will soon remove the support > for Mac below 10.8, so we can just reland this by skipping tests on the failed > Mac platform. See: > http://chrome.blogspot.com/2015/11/updates-to-chrome-platform-support.html The failure I saw was on 10.6, but it doesn't really make sense why the mac version would matter. It seems more plausible that we are not building minidump_stackwalk for some build configurations. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
