Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184263004:
Integration test for minidump paths, adding in wait time to make sure the (Closed)
DescriptionIntegration test for minidump paths, adding in wait time to make sure the
browser has been fully loaded before crashing it.
BUG=catapult:#2346
Committed: https://crrev.com/5c5493a6115dce05a808eed392ac15345d1d3129
Cr-Commit-Position: refs/heads/master@{#408628}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Updating method name #Patch Set 3 : Not hitting a live network #Patch Set 4 : reverting platform change #Messages
Total messages: 20 (10 generated)
eyaich@google.com changed reviewers: + nednguyen@google.com
The CQ bit was checked by eyaich@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm with nits https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... File tools/perf/core/minidump_unittest.py (right): https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... tools/perf/core/minidump_unittest.py:103: def _loadPageAndWait(self, script, value): nits: _LoadPageThenWait(self, script, value): Our convention for method names in python is "DoThisOperation." https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... tools/perf/core/minidump_unittest.py:105: new_tab.Navigate('http://www.google.com/', http://www.google.com would probably hit real network. You probably wants to use "about:blank" or static page instead.
https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... File tools/perf/core/minidump_unittest.py (right): https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... tools/perf/core/minidump_unittest.py:103: def _loadPageAndWait(self, script, value): On 2016/07/28 15:44:47, nednguyen wrote: > nits: _LoadPageThenWait(self, script, value): > > Our convention for method names in python is "DoThisOperation." Done. https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... tools/perf/core/minidump_unittest.py:105: new_tab.Navigate('http://www.google.com/', On 2016/07/28 15:44:48, nednguyen wrote: > http://www.google.com would probably hit real network. You probably wants to use > "about:blank" or static page instead. I think we actually want to hit a real web page and not just about:blank. We want the browser to fully load and therefore establish all tmp dirs. I have a hunch, that may be way off, but I was seeing that the browser was sometimes flaky on the first crash and we couldn't find the first minidump path. I was wondering if it was because the browser was crashed before everything was setup, hence why I am waiting for at least one full page to load first.
https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... File tools/perf/core/minidump_unittest.py (right): https://codereview.chromium.org/2184263004/diff/1/tools/perf/core/minidump_un... tools/perf/core/minidump_unittest.py:105: new_tab.Navigate('http://www.google.com/', On 2016/07/28 15:55:52, eyaich wrote: > On 2016/07/28 15:44:48, nednguyen wrote: > > http://www.google.com would probably hit real network. You probably wants to > use > > "about:blank" or static page instead. > > I think we actually want to hit a real web page and not just about:blank. We > want the browser to fully load and therefore establish all tmp dirs. > > I have a hunch, that may be way off, but I was seeing that the browser was > sometimes flaky on the first crash and we couldn't find the first minidump path. > I was wondering if it was because the browser was crashed before everything > was setup, hence why I am waiting for at least one full page to load first. Talked offline, I have changed this now. Done.
lgtm
The CQ bit was checked by eyaich@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eyaich@google.com
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/2184263004/#ps60001 (title: "reverting platform change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Integration test for minidump paths, adding in wait time to make sure the browser has been fully loaded before crashing it. BUG=catapult:#2346 ========== to ========== Integration test for minidump paths, adding in wait time to make sure the browser has been fully loaded before crashing it. BUG=catapult:#2346 Committed: https://crrev.com/5c5493a6115dce05a808eed392ac15345d1d3129 Cr-Commit-Position: refs/heads/master@{#408628} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5c5493a6115dce05a808eed392ac15345d1d3129 Cr-Commit-Position: refs/heads/master@{#408628} |
