Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767143003:
Take desktop screenshots after test failures in test_runner.py. (Closed)
DescriptionTake desktop screenshots after test failures in test_runner.py.
There have been some simulator issues that are hard to debug, where
the simulator is black/blank, or there are system alerts, which the
simulator doesn't have access to in screenshots. Having desktop
screenshots would be a great debugging aid.
BUG=704138
Review-Url: https://codereview.chromium.org/2767143003
Cr-Commit-Position: refs/heads/master@{#459356}
Committed: https://chromium.googlesource.com/chromium/src/+/54548ddbe9febbe12dafb3178811a0c86b0636f7
Patch Set 1 #
Total comments: 3
Patch Set 2 : address comments #Patch Set 3 : count failures #Patch Set 4 : take screenshot at arbitrary intervals #Patch Set 5 : screenshots for failures #Patch Set 6 : comment #
Total comments: 1
Patch Set 7 : better comment #Messages
Total messages: 32 (12 generated)
baxley@chromium.org changed reviewers: + huangml@google.com, smut@google.com
Let me know what you think. This may be a bit hacky, so I'm totally open to alternatives. We're currently having some failures with black simulators, and this could help debug what is really going on. The current desktop screenshots happen after the simulator is being killed.
https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... File ios/build/bots/scripts/test_runner.py (right): https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() parser is reinitialized every time (lines 279-282) so on line 291 failed_test_count will always be zero. Line 301 can then be "if parser.FailedTests():".
Thanks! https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... File ios/build/bots/scripts/test_runner.py (right): https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() On 2017/03/22 22:46:54, smut wrote: > parser is reinitialized every time (lines 279-282) so on line 291 > failed_test_count will always be zero. Line 301 can then be "if > parser.FailedTests():". Done.
lgtm
The CQ bit was checked by baxley@chromium.org
The CQ bit was checked by baxley@chromium.org
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by baxley@chromium.org
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
let me know what you think! https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... File ios/build/bots/scripts/test_runner.py (right): https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() On 2017/03/22 23:24:39, baxley wrote: > On 2017/03/22 22:46:54, smut wrote: > > parser is reinitialized every time (lines 279-282) so on line 291 > > failed_test_count will always be zero. Line 301 can then be "if > > parser.FailedTests():". > > Done. I don't think this works. parser is initialized outside of the while loop, so once there is a test failure then a screenshot is taken for every line of output. When running ios-simulator try job it fails due to not enough space. I reverted to the mode where I count failures and only take a screenshot when it increases. Is the documents directory cleaned up after each suite on simulator (I thought it was, but want to confirm). This will store on the order of 1MB per screenshot.
Description was changed from ========== Take desktop screenshots after test failures. There have been some simulator issues that are hard to debug, where the simulator is black/blank, or there are system alerts, which the simulator doesn't have access to in screenshots. Having desktop screenshots would be a great debugging aid. BUG=704138 ========== to ========== Take desktop screenshots every 1000 lines of output in test_runner. Limit the total number of screenshots taken while output is being printed to 6. There have been some simulator issues that are hard to debug, where the simulator is black/blank, or there are system alerts, which the simulator doesn't have access to in screenshots. Having desktop screenshots would be a great debugging aid. BUG=704138 ==========
On 2017/03/23 13:54:20, baxley wrote: > let me know what you think! > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > File ios/build/bots/scripts/test_runner.py (right): > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > On 2017/03/22 23:24:39, baxley wrote: > > On 2017/03/22 22:46:54, smut wrote: > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > failed_test_count will always be zero. Line 301 can then be "if > > > parser.FailedTests():". > > > > Done. > > I don't think this works. parser is initialized outside of the while loop, so > once there is a test failure then a screenshot is taken for every line of > output. When running ios-simulator try job it fails due to not enough space. I > reverted to the mode where I count failures and only take a screenshot when it > increases. > > Is the documents directory cleaned up after each suite on simulator (I thought > it was, but want to confirm). This will store on the order of 1MB per > screenshot. Sorry, I didn't realize you were trying to take a screenshot of each individual test case. I thought you wanted one final screenshot of the simulator taken before the simulator was killed, because the current behavior takes the one screenshot after the simulator is killed.
On 2017/03/23 23:25:21, smut wrote: > On 2017/03/23 13:54:20, baxley wrote: > > let me know what you think! > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > File ios/build/bots/scripts/test_runner.py (right): > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > > On 2017/03/22 23:24:39, baxley wrote: > > > On 2017/03/22 22:46:54, smut wrote: > > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > > failed_test_count will always be zero. Line 301 can then be "if > > > > parser.FailedTests():". > > > > > > Done. > > > > I don't think this works. parser is initialized outside of the while loop, so > > once there is a test failure then a screenshot is taken for every line of > > output. When running ios-simulator try job it fails due to not enough space. I > > reverted to the mode where I count failures and only take a screenshot when it > > increases. > > > > Is the documents directory cleaned up after each suite on simulator (I thought > > it was, but want to confirm). This will store on the order of 1MB per > > screenshot. > > Sorry, I didn't realize you were trying to take a screenshot of each individual > test case. I thought you wanted one final screenshot of the simulator taken > before the simulator was killed, because the current behavior takes the one > screenshot after the simulator is killed. A couple comments on screenshotting every failure: 1. Aren't the tests themselves already getting these per-failure screenshots? 2. It's likely that by the time the screenshot is taken by test_runner.py that the test suite has moved on to the next test case. 3. If you really want to try to get per-screenshot failures despite #1 and #2, why doesn't patchset #3 work?
On 2017/03/23 23:32:24, smut wrote: > On 2017/03/23 23:25:21, smut wrote: > > On 2017/03/23 13:54:20, baxley wrote: > > > let me know what you think! > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > File ios/build/bots/scripts/test_runner.py (right): > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > > > On 2017/03/22 23:24:39, baxley wrote: > > > > On 2017/03/22 22:46:54, smut wrote: > > > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > > > failed_test_count will always be zero. Line 301 can then be "if > > > > > parser.FailedTests():". > > > > > > > > Done. > > > > > > I don't think this works. parser is initialized outside of the while loop, > so > > > once there is a test failure then a screenshot is taken for every line of > > > output. When running ios-simulator try job it fails due to not enough space. > I > > > reverted to the mode where I count failures and only take a screenshot when > it > > > increases. > > > > > > Is the documents directory cleaned up after each suite on simulator (I > thought > > > it was, but want to confirm). This will store on the order of 1MB per > > > screenshot. > > > > Sorry, I didn't realize you were trying to take a screenshot of each > individual > > test case. I thought you wanted one final screenshot of the simulator taken > > before the simulator was killed, because the current behavior takes the one > > screenshot after the simulator is killed. > > A couple comments on screenshotting every failure: > 1. Aren't the tests themselves already getting these per-failure screenshots? Yes, but these are from the simulator. For a certain class of failure we don't trust the simulator. It is returning blank/black screenshots. The log shows a view hierarchy, however the test is failing due to things not being visible, which the hierarchy is showing. We want desktop screenshots to try to understand what is going on. > 2. It's likely that by the time the screenshot is taken by test_runner.py that > the test suite has moved on to the next test case. Yeah, it's not super critical we get it at the test case. For this case, every test is failing with the problem I mentioned above. We just want to see the state of the simulator. > 3. If you really want to try to get per-screenshot failures despite #1 and #2, > why doesn't patchset #3 work? I got nervous by the comment from parser.FailedTests(), that it reports running test as failed. I guess if we're storing the result it will only grow when it increases, meaning patch 3 should work. If you're confident that would work, I'm happy to go with that. Thanks!
On 2017/03/23 23:25:21, smut wrote: > On 2017/03/23 13:54:20, baxley wrote: > > let me know what you think! > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > File ios/build/bots/scripts/test_runner.py (right): > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > > On 2017/03/22 23:24:39, baxley wrote: > > > On 2017/03/22 22:46:54, smut wrote: > > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > > failed_test_count will always be zero. Line 301 can then be "if > > > > parser.FailedTests():". > > > > > > Done. > > > > I don't think this works. parser is initialized outside of the while loop, so > > once there is a test failure then a screenshot is taken for every line of > > output. When running ios-simulator try job it fails due to not enough space. I > > reverted to the mode where I count failures and only take a screenshot when it > > increases. > > > > Is the documents directory cleaned up after each suite on simulator (I thought > > it was, but want to confirm). This will store on the order of 1MB per > > screenshot. > > Sorry, I didn't realize you were trying to take a screenshot of each individual > test case. I thought you wanted one final screenshot of the simulator taken > before the simulator was killed, because the current behavior takes the one > screenshot after the simulator is killed. That could work. It would be nice to get screenshots during the test, but one screenshot before and after the tests are run might help.
On 2017/03/23 23:42:36, baxley wrote: > On 2017/03/23 23:32:24, smut wrote: > > On 2017/03/23 23:25:21, smut wrote: > > > On 2017/03/23 13:54:20, baxley wrote: > > > > let me know what you think! > > > > > > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > > File ios/build/bots/scripts/test_runner.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > > > > On 2017/03/22 23:24:39, baxley wrote: > > > > > On 2017/03/22 22:46:54, smut wrote: > > > > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > > > > failed_test_count will always be zero. Line 301 can then be "if > > > > > > parser.FailedTests():". > > > > > > > > > > Done. > > > > > > > > I don't think this works. parser is initialized outside of the while loop, > > so > > > > once there is a test failure then a screenshot is taken for every line of > > > > output. When running ios-simulator try job it fails due to not enough > space. > > I > > > > reverted to the mode where I count failures and only take a screenshot > when > > it > > > > increases. > > > > > > > > Is the documents directory cleaned up after each suite on simulator (I > > thought > > > > it was, but want to confirm). This will store on the order of 1MB per > > > > screenshot. > > > > > > Sorry, I didn't realize you were trying to take a screenshot of each > > individual > > > test case. I thought you wanted one final screenshot of the simulator taken > > > before the simulator was killed, because the current behavior takes the one > > > screenshot after the simulator is killed. > > > > A couple comments on screenshotting every failure: > > 1. Aren't the tests themselves already getting these per-failure screenshots? > Yes, but these are from the simulator. For a certain class of failure we don't > trust the simulator. It is returning blank/black screenshots. The log shows a > view hierarchy, however the test is failing due to things not being visible, > which the hierarchy is showing. We want desktop screenshots to try to understand > what is going on. > > > 2. It's likely that by the time the screenshot is taken by test_runner.py that > > the test suite has moved on to the next test case. > Yeah, it's not super critical we get it at the test case. For this case, every > test is failing with the problem I mentioned above. We just want to see the > state of the simulator. > > > 3. If you really want to try to get per-screenshot failures despite #1 and #2, > > why doesn't patchset #3 work? > I got nervous by the comment from parser.FailedTests(), that it reports running > test as failed. I guess if we're storing the result it will only grow when it > increases, meaning patch 3 should work. If you're confident that would work, I'm > happy to go with that. > > Thanks! Got it. In that case, patch #3 won't work. You could use len([test for test in parser.FailedTests() if test != parser.GetCurrentTest()]) as the count of failures, and if that count increases then you take a screenshot. Or GTestParser and XCTestParser could be extended to support a failed test callback that they invoke when they first parse the failure.
On 2017/03/24 00:05:50, smut wrote: > On 2017/03/23 23:42:36, baxley wrote: > > On 2017/03/23 23:32:24, smut wrote: > > > On 2017/03/23 23:25:21, smut wrote: > > > > On 2017/03/23 13:54:20, baxley wrote: > > > > > let me know what you think! > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > > > File ios/build/bots/scripts/test_runner.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > > > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > > > > > On 2017/03/22 23:24:39, baxley wrote: > > > > > > On 2017/03/22 22:46:54, smut wrote: > > > > > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > > > > > failed_test_count will always be zero. Line 301 can then be "if > > > > > > > parser.FailedTests():". > > > > > > > > > > > > Done. > > > > > > > > > > I don't think this works. parser is initialized outside of the while > loop, > > > so > > > > > once there is a test failure then a screenshot is taken for every line > of > > > > > output. When running ios-simulator try job it fails due to not enough > > space. > > > I > > > > > reverted to the mode where I count failures and only take a screenshot > > when > > > it > > > > > increases. > > > > > > > > > > Is the documents directory cleaned up after each suite on simulator (I > > > thought > > > > > it was, but want to confirm). This will store on the order of 1MB per > > > > > screenshot. > > > > > > > > Sorry, I didn't realize you were trying to take a screenshot of each > > > individual > > > > test case. I thought you wanted one final screenshot of the simulator > taken > > > > before the simulator was killed, because the current behavior takes the > one > > > > screenshot after the simulator is killed. > > > > > > A couple comments on screenshotting every failure: > > > 1. Aren't the tests themselves already getting these per-failure > screenshots? > > Yes, but these are from the simulator. For a certain class of failure we don't > > trust the simulator. It is returning blank/black screenshots. The log shows a > > view hierarchy, however the test is failing due to things not being visible, > > which the hierarchy is showing. We want desktop screenshots to try to > understand > > what is going on. > > > > > 2. It's likely that by the time the screenshot is taken by test_runner.py > that > > > the test suite has moved on to the next test case. > > Yeah, it's not super critical we get it at the test case. For this case, every > > test is failing with the problem I mentioned above. We just want to see the > > state of the simulator. > > > > > 3. If you really want to try to get per-screenshot failures despite #1 and > #2, > > > why doesn't patchset #3 work? > > I got nervous by the comment from parser.FailedTests(), that it reports > running > > test as failed. I guess if we're storing the result it will only grow when it > > increases, meaning patch 3 should work. If you're confident that would work, > I'm > > happy to go with that. > > > > Thanks! > > Got it. > > In that case, patch #3 won't work. You could use len([test for test in > parser.FailedTests() if test != parser.GetCurrentTest()]) as the count of > failures, and if that count increases then you take a screenshot. Or GTestParser > and XCTestParser could be extended to support a failed test callback that they > invoke when they first parse the failure. This is something we'd like to have ASAP to debug some current problems. Is it okay to go with your first approach? I'm happy to do that, then revert it or replace it with a better solution, if this is something we find is useful.
On 2017/03/24 00:57:56, baxley wrote: > On 2017/03/24 00:05:50, smut wrote: > > On 2017/03/23 23:42:36, baxley wrote: > > > On 2017/03/23 23:32:24, smut wrote: > > > > On 2017/03/23 23:25:21, smut wrote: > > > > > On 2017/03/23 13:54:20, baxley wrote: > > > > > > let me know what you think! > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > > > > File ios/build/bots/scripts/test_runner.py (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2767143003/diff/1/ios/build/bots/scripts/test... > > > > > > ios/build/bots/scripts/test_runner.py:305: self.screenshot_desktop() > > > > > > On 2017/03/22 23:24:39, baxley wrote: > > > > > > > On 2017/03/22 22:46:54, smut wrote: > > > > > > > > parser is reinitialized every time (lines 279-282) so on line 291 > > > > > > > > failed_test_count will always be zero. Line 301 can then be "if > > > > > > > > parser.FailedTests():". > > > > > > > > > > > > > > Done. > > > > > > > > > > > > I don't think this works. parser is initialized outside of the while > > loop, > > > > so > > > > > > once there is a test failure then a screenshot is taken for every line > > of > > > > > > output. When running ios-simulator try job it fails due to not enough > > > space. > > > > I > > > > > > reverted to the mode where I count failures and only take a screenshot > > > when > > > > it > > > > > > increases. > > > > > > > > > > > > Is the documents directory cleaned up after each suite on simulator (I > > > > thought > > > > > > it was, but want to confirm). This will store on the order of 1MB per > > > > > > screenshot. > > > > > > > > > > Sorry, I didn't realize you were trying to take a screenshot of each > > > > individual > > > > > test case. I thought you wanted one final screenshot of the simulator > > taken > > > > > before the simulator was killed, because the current behavior takes the > > one > > > > > screenshot after the simulator is killed. > > > > > > > > A couple comments on screenshotting every failure: > > > > 1. Aren't the tests themselves already getting these per-failure > > screenshots? > > > Yes, but these are from the simulator. For a certain class of failure we > don't > > > trust the simulator. It is returning blank/black screenshots. The log shows > a > > > view hierarchy, however the test is failing due to things not being visible, > > > which the hierarchy is showing. We want desktop screenshots to try to > > understand > > > what is going on. > > > > > > > 2. It's likely that by the time the screenshot is taken by test_runner.py > > that > > > > the test suite has moved on to the next test case. > > > Yeah, it's not super critical we get it at the test case. For this case, > every > > > test is failing with the problem I mentioned above. We just want to see the > > > state of the simulator. > > > > > > > 3. If you really want to try to get per-screenshot failures despite #1 and > > #2, > > > > why doesn't patchset #3 work? > > > I got nervous by the comment from parser.FailedTests(), that it reports > > running > > > test as failed. I guess if we're storing the result it will only grow when > it > > > increases, meaning patch 3 should work. If you're confident that would work, > > I'm > > > happy to go with that. > > > > > > Thanks! > > > > Got it. > > > > In that case, patch #3 won't work. You could use len([test for test in > > parser.FailedTests() if test != parser.GetCurrentTest()]) as the count of > > failures, and if that count increases then you take a screenshot. Or > GTestParser > > and XCTestParser could be extended to support a failed test callback that they > > invoke when they first parse the failure. > > This is something we'd like to have ASAP to debug some current problems. Is it > okay to go with your first approach? I'm happy to do that, then revert it or > replace it with a better solution, if this is something we find is useful. First approach is fine.
Description was changed from
==========
Take desktop screenshots every 1000 lines of output in test_runner.
Limit the total number of screenshots taken while output is being printed
to 6.
There have been some simulator issues that are hard to debug, where
the simulator is black/blank, or there are system alerts, which the
simulator doesn't have access to in screenshots. Having desktop
screenshots would be a great debugging aid.
BUG=704138
==========
to
==========
Take desktop screenshots after test failures in test_runner.py.
There have been some simulator issues that are hard to debug, where
the simulator is black/blank, or there are system alerts, which the
simulator doesn't have access to in screenshots. Having desktop
screenshots would be a great debugging aid.
BUG=704138
==========
Switched back to screenshots per failure, based on your suggestion. PTAL!
lgtm https://codereview.chromium.org/2767143003/diff/100001/ios/build/bots/scripts... File ios/build/bots/scripts/test_runner.py (right): https://codereview.chromium.org/2767143003/diff/100001/ios/build/bots/scripts... ios/build/bots/scripts/test_runner.py:301: # If there is a new test failure, take a desktop screenshot. Could add a comment explaining that the parser considers the current test to be failed until it completes successfully.
The CQ bit was checked by baxley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from smut@google.com Link to the patchset: https://codereview.chromium.org/2767143003/#ps80002 (title: "better comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80002, "attempt_start_ts": 1490324397912930,
"parent_rev": "2c9c4cbd20c337d4c895037f39fb15a8560bc32c", "commit_rev":
"54548ddbe9febbe12dafb3178811a0c86b0636f7"}
Message was sent while issue was closed.
Description was changed from
==========
Take desktop screenshots after test failures in test_runner.py.
There have been some simulator issues that are hard to debug, where
the simulator is black/blank, or there are system alerts, which the
simulator doesn't have access to in screenshots. Having desktop
screenshots would be a great debugging aid.
BUG=704138
==========
to
==========
Take desktop screenshots after test failures in test_runner.py.
There have been some simulator issues that are hard to debug, where
the simulator is black/blank, or there are system alerts, which the
simulator doesn't have access to in screenshots. Having desktop
screenshots would be a great debugging aid.
BUG=704138
Review-Url: https://codereview.chromium.org/2767143003
Cr-Commit-Position: refs/heads/master@{#459356}
Committed:
https://chromium.googlesource.com/chromium/src/+/54548ddbe9febbe12dafb3178811...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:80002) as https://chromium.googlesource.com/chromium/src/+/54548ddbe9febbe12dafb3178811... |
