Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
430473011:
Fleshout cloud_storage stub implementation. (Closed)
|
Created:
6 years, 4 months ago by aiolos (Not reviewing) Modified:
6 years, 2 months ago Reviewers:
dtu CC:
chromium-reviews, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFleshout cloud_storage stub implementation.
BUG=
Committed: https://crrev.com/7d9213cbeae18af0e0fc72699e99b461eafcfa76
Cr-Commit-Position: refs/heads/master@{#297762}
Patch Set 1 #Patch Set 2 : Updated CloudStorageStub and added unittests, in preperation for record_wpr testing. #
Total comments: 18
Patch Set 3 : dtu comments. #Patch Set 4 : Remove presubmit unittests since presubmit is going to be deleted in hte next CL. #
Messages
Total messages: 16 (4 generated)
aiolos@chromium.org changed reviewers: + dtu@chromium.org
I didn't do a completely thorough unittest pass on for this, but didn't think it needed more for a testing stub. Let me know if you disagree. I wrote the stub to mirror that in cloud_storage.py. It seemed slightly odd to me that Get will throw a NotFound error, but GetIfChanged won't. Is there a reason for that?
On 2014/09/11 17:53:16, aiolos wrote: > I didn't do a completely thorough unittest pass on for this, but didn't think it > needed more for a testing stub. Let me know if you disagree. > > I wrote the stub to mirror that in cloud_storage.py. It seemed slightly odd to > me that Get will throw a NotFound error, but GetIfChanged won't. Is there a > reason for that? Yes! It's a wart that comes from the WPR recording workflow. When a user records a WPR archive locally and runs from their local archive, we don't want to spew errors that it couldn't update the file from Cloud Storage - it was never supposed to be in Cloud Storage. I think the new workflow will help with this - if the user chooses not to upload, don't generate a .sha1 file, and we will skip this check.
https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... File tools/telemetry/telemetry/unittest/system_stub.py (right): https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:126: # These must match those in telemetry.util.cloud_storage Can we use unique but fake names? Just an extra sanity check that we're not hitting the live server. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:129: PUBLIC_BUCKET = 'chromium-telemetry' I added a BUCKET_ALIASES field and you may need to rebase. nit: Also use the same ordering as cloud_storage.py. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:163: if (bucket == CloudStorageModuleStub.PUBLIC_BUCKET): nit: style guide: remove extra (). Same throughout this file. This ain't C++ :P https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:205: def List(self, bucket=None): The API requires a bucket parameter and disallows listing multiple buckets with one call. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:210: bucket_error = 'Incorrect bucket specified, correct buckets:' +\ nit: style guide: use () instead of \. Same below. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:228: raise CloudStorageModuleStub.CloudStorageError(file_path_error) nit: I think this is an OSError. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... File tools/telemetry/telemetry/unittest/system_stub_unittest.py (right): https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub_unittest.py:10: sys.path.insert(0, os.path.join(os.path.dirname(PERF_ROOT), 'telemetry')) Remove. (See comment at bottom of file.) https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub_unittest.py:16: PUBLIC_FILE_HASH = 'publicee5e6b4b0d3255bfef95601890afd80709' nit: Can you use zfill to make it immediately obvious these aren't real hashes and that they're all unique? https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub_unittest.py:223: unittest.main() nit: We haven't been using these, so for consistency, remove? (I think the argument against being able to run unit tests directly is slightly stronger than for them.)
https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... File tools/telemetry/telemetry/unittest/system_stub.py (right): https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:126: # These must match those in telemetry.util.cloud_storage On 2014/10/01 00:15:28, dtu wrote: > Can we use unique but fake names? Just an extra sanity check that we're not > hitting the live server. But won't those names be passed in from record_wpr and other tools?
https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... File tools/telemetry/telemetry/unittest/system_stub.py (right): https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:129: PUBLIC_BUCKET = 'chromium-telemetry' On 2014/10/01 00:15:28, dtu wrote: > I added a BUCKET_ALIASES field and you may need to rebase. > nit: Also use the same ordering as cloud_storage.py. Done. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:163: if (bucket == CloudStorageModuleStub.PUBLIC_BUCKET): On 2014/10/01 00:15:28, dtu wrote: > nit: style guide: remove extra (). Same throughout this file. This ain't C++ :P Done. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:205: def List(self, bucket=None): On 2014/10/01 00:15:28, dtu wrote: > The API requires a bucket parameter and disallows listing multiple buckets with > one call. Done. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:210: bucket_error = 'Incorrect bucket specified, correct buckets:' +\ On 2014/10/01 00:15:28, dtu wrote: > nit: style guide: use () instead of \. Same below. Done. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... File tools/telemetry/telemetry/unittest/system_stub_unittest.py (right): https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub_unittest.py:10: sys.path.insert(0, os.path.join(os.path.dirname(PERF_ROOT), 'telemetry')) On 2014/10/01 00:15:28, dtu wrote: > Remove. (See comment at bottom of file.) Done. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub_unittest.py:16: PUBLIC_FILE_HASH = 'publicee5e6b4b0d3255bfef95601890afd80709' On 2014/10/01 00:15:28, dtu wrote: > nit: Can you use zfill to make it immediately obvious these aren't real hashes > and that they're all unique? Done. https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub_unittest.py:223: unittest.main() On 2014/10/01 00:15:28, dtu wrote: > nit: We haven't been using these, so for consistency, remove? (I think the > argument against being able to run unit tests directly is slightly stronger than > for them.) Done.
https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... File tools/telemetry/telemetry/unittest/system_stub.py (right): https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... tools/telemetry/telemetry/unittest/system_stub.py:126: # These must match those in telemetry.util.cloud_storage On 2014/10/01 01:24:41, aiolos wrote: > On 2014/10/01 00:15:28, dtu wrote: > > Can we use unique but fake names? Just an extra sanity check that we're not > > hitting the live server. > > But won't those names be passed in from record_wpr and other tools? record_wpr gets the bucket names from the page set. If you want me to make a fake PageSet when I'm writing the unittests for it, these can change. Otherwise, they shouldn't. Preference?
On 2014/10/01 17:53:59, aiolos wrote: > https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... > File tools/telemetry/telemetry/unittest/system_stub.py (right): > > https://chromiumcodereview.appspot.com/430473011/diff/20001/tools/telemetry/t... > tools/telemetry/telemetry/unittest/system_stub.py:126: # These must match those > in telemetry.util.cloud_storage > On 2014/10/01 01:24:41, aiolos wrote: > > On 2014/10/01 00:15:28, dtu wrote: > > > Can we use unique but fake names? Just an extra sanity check that we're not > > > hitting the live server. > > > > But won't those names be passed in from record_wpr and other tools? > > record_wpr gets the bucket names from the page set. If you want me to make a > fake PageSet when I'm writing the unittests for it, these can change. Otherwise, > they shouldn't. Preference? It's fine as is. lgtm The page sets get their bucket names from cloud_storage constants, but it might be too annoying to stub out cloud_storage in every module.
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/430473011/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/430473011/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 79fca874a4da3e1a7485d32f935b875ccc937ecd
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7d9213cbeae18af0e0fc72699e99b461eafcfa76 Cr-Commit-Position: refs/heads/master@{#297762} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
