Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
834173006:
[Telemetry] Fix WPR files not being downloaded when required. (Closed)
Description[Telemetry] Fix WPR files not being downloaded when required.
BUG=
Committed: https://crrev.com/672e7140315568e9bfc442770a23fee66e045cb2
Cr-Commit-Position: refs/heads/master@{#311496}
Patch Set 1 #Patch Set 2 : Unittest #
Total comments: 2
Patch Set 3 : Remove useless check. #
Total comments: 6
Patch Set 4 : Only use the CloudStorage mock in one test, as it breaks the others. #
Messages
Total messages: 36 (9 generated)
lizeb@chromium.org changed reviewers: + nednguyen@google.com
nednguyen@google.com changed reviewers: + aiolos@chromium.org
Can we add unittest for this? You can use cloud_storage stub in unittest_util/system_stub.py to avoid invoking real network.
On 2015/01/13 15:31:31, nednguyen wrote: > Can we add unittest for this? You can use cloud_storage stub in > unittest_util/system_stub.py to avoid invoking real network. Done. I'm not familiar with this code, so I'm not that confident with the unittest.
https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info_unittest.py (right): https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info_unittest.py:77: Can you add another test for the case archive_info gets the empty data?
https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info_unittest.py (right): https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info_unittest.py:77: On 2015/01/13 17:50:22, nednguyen wrote: > Can you add another test for the case archive_info gets the empty data? Before this commit, the code would crash with bool(WprArchiveInfo._data) == False. Also, it is not possible to get WprArchiveInfo._data == None (or any false value) using WprArchiveInfo.FromFile, which is the only way it is constructed in the tests and in the code. So it would probably make more sense to add assert(self._data) in the code than to add a test. What do you think ?
On 2015/01/13 17:58:33, lizeb wrote: > https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/wpr/archive_info_unittest.py (right): > > https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/wpr/archive_info_unittest.py:77: > On 2015/01/13 17:50:22, nednguyen wrote: > > Can you add another test for the case archive_info gets the empty data? > > Before this commit, the code would crash with bool(WprArchiveInfo._data) == > False. Also, it is not possible to get WprArchiveInfo._data == None (or any > false value) using WprArchiveInfo.FromFile, which is the only way it is > constructed in the tests and in the code. What if we have a file that contains empty json dict or list? i.e: '{}', '[]'? > > So it would probably make more sense to add > assert(self._data) > in the code than to add a test. > > What do you think ?
On 2015/01/13 18:02:27, nednguyen wrote: > On 2015/01/13 17:58:33, lizeb wrote: > > > https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/wpr/archive_info_unittest.py (right): > > > > > https://codereview.chromium.org/834173006/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/wpr/archive_info_unittest.py:77: > > On 2015/01/13 17:50:22, nednguyen wrote: > > > Can you add another test for the case archive_info gets the empty data? > > > > Before this commit, the code would crash with bool(WprArchiveInfo._data) == > > False. Also, it is not possible to get WprArchiveInfo._data == None (or any > > false value) using WprArchiveInfo.FromFile, which is the only way it is > > constructed in the tests and in the code. > > What if we have a file that contains empty json dict or list? i.e: '{}', '[]'? > Actually, you can't construct the object in this case. I've removed the useless check from WprArchiveInfo.DownloadArchivesIfNeeded.
lgtm https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info.py (right): https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info.py:78: 'cannot be downloaded from cloud_storage.', ) Can you add: assert 'archives' in self._data, "Invalid data from file %s' % self._file_path ?
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/834173006/40001
https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info.py (right): https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info.py:78: 'cannot be downloaded from cloud_storage.', ) On 2015/01/13 18:20:59, nednguyen wrote: > Can you add: assert 'archives' in self._data, "Invalid data from file %s' % > self._file_path ? Acknowledged. The constructor would throw without 'archives' in self._data.
https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info.py (right): https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info.py:78: 'cannot be downloaded from cloud_storage.', ) On 2015/01/13 18:24:39, lizeb wrote: > On 2015/01/13 18:20:59, nednguyen wrote: > > Can you add: assert 'archives' in self._data, "Invalid data from file %s' % > > self._file_path ? > > Acknowledged. > > The constructor would throw without 'archives' in self._data. I prefer assertion since it gives a clearer debugging message. Also you get to know the name of the file that contains invalid data that debug message.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info.py (left): https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info.py:76: if self._data: I believe this should actually be if not self._data: Because we assume self.data_ in the next code section.
https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info.py (left): https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info.py:76: if self._data: On 2015/01/13 19:42:58, aiolos wrote: > I believe this should actually be > > if not self._data: > > Because we assume self.data_ in the next code section. lizeb@'s point is self._data cannot be None since FromFile is the constructor method to be used. If it's just empty, then self._data['archives'] will throw exception. Anyway, I asked lizeb@ to add the assertion that 'archives' in self._data.
https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/wpr/archive_info.py (left): https://codereview.chromium.org/834173006/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/wpr/archive_info.py:76: if self._data: On 2015/01/13 19:58:51, nednguyen wrote: > On 2015/01/13 19:42:58, aiolos wrote: > > I believe this should actually be > > > > if not self._data: > > > > Because we assume self.data_ in the next code section. > > lizeb@'s point is self._data cannot be None since FromFile is the constructor > method to be used. If it's just empty, then self._data['archives'] will throw > exception. Anyway, I asked lizeb@ to add the assertion that 'archives' in > self._data. Ah, I missed that comment. We might rely on this not throwing exceptions on archives with no data for some testing bits. If it fails in the CQ, we should look into whether any of those assumption are valid, and may need to adjust some tests. The name of the function is slightly misleading if we added the assertion, and should at the very least have it's doc updated to reflect that it will fail the assert if self.data_ is empty.
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/834173006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/01/13 22:22:47, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Seems like you have a legit failure. Also can you address Karin's concern?
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/834173006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
On 2015/01/13 23:02:07, nednguyen wrote: > On 2015/01/13 22:22:47, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Seems like you have a legit failure. Also can you address Karin's concern? The failure was due to the other tests failing with the mock CloudStorage. It is fixed. As for the assertion, it is impossible to construct the object without a valid _data, as both the constructor __init__ and the class method FromFile assume: 1. data is not empty 2. data['archives'] exists.
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/834173006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/672e7140315568e9bfc442770a23fee66e045cb2 Cr-Commit-Position: refs/heads/master@{#311496}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/852863003/ by rockot@chromium.org. The reason for reverting is: Possible suspect for newly introduced telemetry unit test flake, e.g.: https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2... Reverting to see what happens..
Message was sent while issue was closed.
On 2015/01/14 22:07:56, Ken Rockot wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/852863003/ by mailto:rockot@chromium.org. > > The reason for reverting is: Possible suspect for newly introduced telemetry > unit test flake, e.g.: > > https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2... > > Reverting to see what happens.. See also crbug/449186 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
