Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626373004:
[WPR] Implement platform dependent recordings for WPR archives. (Closed)
|
Created:
3 years, 11 months ago by rnephew (Reviews Here) Modified:
3 years, 10 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[WPR] Implement platform dependent recordings for WPR archives.
BUG=chromium:618698
,catapult:#2814
Review-Url: https://codereview.chromium.org/2626373004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/7af1a1e70eb99a3162fc2a1d82d209468a887a60
Patch Set 1 #Patch Set 2 : Add multiplatform plumbing #Patch Set 3 : rework to not use system platform module because of naming conflict within telemetry #Patch Set 4 : [WPR] Implement platform dependent recordings for WPR archives. #
Total comments: 8
Patch Set 5 : rename multiplatform and change how platform is detected when adding new recordings plus rebase #
Total comments: 12
Patch Set 6 : [WPR] Implement platform dependent recordings for WPR archives. #
Total comments: 10
Patch Set 7 : [WPR] Implement platform dependent recordings for WPR archives. #Patch Set 8 : fix unittest for download #
Total comments: 30
Patch Set 9 : [WPR] Implement platform dependent recordings for WPR archives. #
Total comments: 20
Patch Set 10 : [WPR] Implement platform dependent recordings for WPR archives. #
Total comments: 2
Patch Set 11 : Rebase and Juans suggestion #Patch Set 12 : [WPR] Implement platform dependent recordings for WPR archives. #Patch Set 13 : [WPR] Implement platform dependent recordings for WPR archives. #
Total comments: 8
Patch Set 14 : neds suggestions #
Total comments: 2
Patch Set 15 : [WPR] Implement platform dependent recordings for WPR archives. #
Total comments: 3
Patch Set 16 : [WPR] Implement platform dependent recordings for WPR archives. #Patch Set 17 : Rebase #Messages
Total messages: 75 (28 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
Patchset #2 (id:20001) has been deleted
Patchset #4 (id:80001) has been deleted
Is there a reason that we need to specify "multiplatform" in the story definition? Why can't we just look in the JSON file and use the platform-specific version if available and fall back to the platform-independent version one if it isn't?
On 2017/01/17 21:12:29, charliea wrote: > Is there a reason that we need to specify "multiplatform" in the story > definition? Why can't we just look in the JSON file and use the > platform-specific version if available and fall back to the platform-independent > version one if it isn't? We specify multiplatform mode so that during the initial recording, it knows if it should be saved as 'Default' or as 'Default' and 'PLATFORM'.
https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/pa... File telemetry/telemetry/page/__init__.py (right): https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/pa... telemetry/telemetry/page/__init__.py:28: multiplatform=False): I'm not a huge fan of "multiplatform" as the name of the parameter: to me, it suggests exactly the opposite of what it means in practice, which is that each recording is expected to only work on one platform. What about "platform_specific"? https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (left): https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:150: def _DeleteAbandonedWprFiles(self): Is this related to this CL? https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. Could you elaborate on what the purpose of this file is? https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:206: # If there is no other recording we want the first to be the default wat?
https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/pa... File telemetry/telemetry/page/__init__.py (right): https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/pa... telemetry/telemetry/page/__init__.py:28: multiplatform=False): On 2017/01/17 21:38:43, charliea wrote: > I'm not a huge fan of "multiplatform" as the name of the parameter: to me, it > suggests exactly the opposite of what it means in practice, which is that each > recording is expected to only work on one platform. What about > "platform_specific"? I'll let ned have a chance to give input on a name before changing, but I agree with multiplatform not being descriptive enough. https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (left): https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:150: def _DeleteAbandonedWprFiles(self): On 2017/01/17 21:38:43, charliea wrote: > Is this related to this CL? The new version doesn't need _DeleteAbandonedWprFiles anymore because of how it was designed. This goes through and deletes empty wpr files from the json file. The new way of storing them automatically does this. This is showing difference between archive_info2.py and archive_info.py, thats why it is showing up here. https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2017/01/17 21:38:43, charliea wrote: > Could you elaborate on what the purpose of this file is? The purpose of this file is the same as archive_info.py. A way to extract data from the archive json files. https://codereview.chromium.org/2626373004/diff/100001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:206: # If there is no other recording we want the first to be the default On 2017/01/17 21:38:43, charliea wrote: > wat? If its the first time a page is recorded, it sets it as the default and the platform(if multiplatform is set).
https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:198: ValidateStory(s) From rebase. https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:298: def ValidateStory(story): From rebase.
https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/st... File telemetry/telemetry/story/story.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/story.py:45: platform_specific: Whether the recordings should be done per platform. I'm not sure I'd understand what this meant if I stumbled across the doc for it. How about something like: platform_specific: Boolean indicating if a separate web page replay recording is required on each platform. https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:16: # TODO(rnephew): Move this file to archive_info.py when it is the default. Could you elaborate on why this is necessary and we can't just work these changes into archive_info.py right now? I'm generally pretty against forking code unless there's a very strong reason to do so. (Regardless, it probably makes sense to document this in the code, too, as other people might wonder the same thing) https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:92: for test in archives: I'm not sure that the thing that is in the archive is a "test" so much as a "story" "story_archive" https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:95: current_platform = sys.platform (here and elsewhere) nednguyen@google.com, didn't we run into problems with using sys.platform and target_platform interchangeably? IIRC, target_platform is sys.platform with some additional manipulation like standardizing things like "linux3_x86", "linux2_x86", etc. to all be "linux_x86". I'm not sure if we make similar replacements, but I know that using these interchangeably before has given us problems, like in https://codereview.chromium.org/2608773002/, which I still need to submit.
https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/st... File telemetry/telemetry/story/story.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/st... telemetry/telemetry/story/story.py:45: platform_specific: Whether the recordings should be done per platform. On 2017/01/18 16:07:58, charliea wrote: > I'm not sure I'd understand what this meant if I stumbled across the doc for it. > How about something like: > > platform_specific: Boolean indicating if a separate web page replay recording is > required on each platform. Done. https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:16: # TODO(rnephew): Move this file to archive_info.py when it is the default. On 2017/01/18 16:07:58, charliea wrote: > Could you elaborate on why this is necessary and we can't just work these > changes into archive_info.py right now? I'm generally pretty against forking > code unless there's a very strong reason to do so. (Regardless, it probably > makes sense to document this in the code, too, as other people might wonder the > same thing) I said more about why. I could have had this be directly part of archive_info.py but it would have been messier with a lot of if/else blocks and harder to ensure that it is doing the correct thing. Since archive_info.py will be deleted relatively shortly (once I update all wpr.json files to the new format) I thought it would be easier to just split them and replace archive_info.py with archive_info2.py after the fact. https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:95: current_platform = sys.platform On 2017/01/18 16:07:58, charliea wrote: > (here and elsewhere) mailto:nednguyen@google.com, didn't we run into problems with > using sys.platform and target_platform interchangeably? IIRC, target_platform is > sys.platform with some additional manipulation like standardizing things like > "linux3_x86", "linux2_x86", etc. to all be "linux_x86". I'm not sure if we make > similar replacements, but I know that using these interchangeably before has > given us problems, like in https://codereview.chromium.org/2608773002/, which I > still need to submit. Waiting on neds feedback on this.
https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:16: # TODO(rnephew): Move this file to archive_info.py when it is the default. On 2017/01/18 18:55:03, rnephew (Reviews Here) wrote: > On 2017/01/18 16:07:58, charliea wrote: > > Could you elaborate on why this is necessary and we can't just work these > > changes into archive_info.py right now? I'm generally pretty against forking > > code unless there's a very strong reason to do so. (Regardless, it probably > > makes sense to document this in the code, too, as other people might wonder > the > > same thing) > > I said more about why. I could have had this be directly part of archive_info.py > but it would have been messier with a lot of if/else blocks and harder to ensure > that it is doing the correct thing. Since archive_info.py will be deleted > relatively shortly (once I update all wpr.json files to the new format) I > thought it would be easier to just split them and replace archive_info.py with > archive_info2.py after the fact. I support this. In this case, it's cleaner to have: If this is the old json: old_object = OldAPI(old_json) old_object.DoFoo() old_object.DoBar() else: new_object = NewAPI(new_json) new_object.DoFoo() new_object.DoBar() instead of: Class OldObject(): .. def DoFoo(): if this is old: Do old stuff elif this is new: Do new stuff def DoBar(): if this is old: Do old stuff elif this is new: Do new stuff https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:95: current_platform = sys.platform On 2017/01/18 18:55:03, rnephew (Reviews Here) wrote: > On 2017/01/18 16:07:58, charliea wrote: > > (here and elsewhere) mailto:nednguyen@google.com, didn't we run into problems > with > > using sys.platform and target_platform interchangeably? IIRC, target_platform > is > > sys.platform with some additional manipulation like standardizing things like > > "linux3_x86", "linux2_x86", etc. to all be "linux_x86". I'm not sure if we > make > > similar replacements, but I know that using these interchangeably before has > > given us problems, like in https://codereview.chromium.org/2608773002/, which > I > > still need to submit. > > Waiting on neds feedback on this. Yeah. Better to use platform provided by callsites instead of inferring it yourself. Remember that Telemetry supports running test on a remote platforms, so the sys.platform value here is could be wrong if Telemetry is running on a Linux host & control the test on Android.
Description was changed from ========== [WPR] Implement platform dependent recordings for WPR archives. BUG=catapult:#2814 ========== to ========== [WPR] Implement platform dependent recordings for WPR archives. BUG=catapult:#2814 ==========
nednguyen@google.com changed reviewers: + perezju@chromium.org
On 2017/01/18 19:30:42, nednguyen wrote: > https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... > File telemetry/telemetry/wpr/archive_info2.py (right): > > https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... > telemetry/telemetry/wpr/archive_info2.py:16: # TODO(rnephew): Move this file to > archive_info.py when it is the default. > On 2017/01/18 18:55:03, rnephew (Reviews Here) wrote: > > On 2017/01/18 16:07:58, charliea wrote: > > > Could you elaborate on why this is necessary and we can't just work these > > > changes into archive_info.py right now? I'm generally pretty against forking > > > code unless there's a very strong reason to do so. (Regardless, it probably > > > makes sense to document this in the code, too, as other people might wonder > > the > > > same thing) > > > > I said more about why. I could have had this be directly part of > archive_info.py > > but it would have been messier with a lot of if/else blocks and harder to > ensure > > that it is doing the correct thing. Since archive_info.py will be deleted > > relatively shortly (once I update all wpr.json files to the new format) I > > thought it would be easier to just split them and replace archive_info.py with > > archive_info2.py after the fact. > > I support this. In this case, it's cleaner to have: > > If this is the old json: > old_object = OldAPI(old_json) > old_object.DoFoo() > old_object.DoBar() > else: > new_object = NewAPI(new_json) > new_object.DoFoo() > new_object.DoBar() > > instead of: > > Class OldObject(): > .. > def DoFoo(): > if this is old: > Do old stuff > elif this is new: > Do new stuff > > def DoBar(): > if this is old: > Do old stuff > elif this is new: > Do new stuff > > https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... > telemetry/telemetry/wpr/archive_info2.py:95: current_platform = sys.platform > On 2017/01/18 18:55:03, rnephew (Reviews Here) wrote: > > On 2017/01/18 16:07:58, charliea wrote: > > > (here and elsewhere) mailto:nednguyen@google.com, didn't we run into > problems > > with > > > using sys.platform and target_platform interchangeably? IIRC, > target_platform > > is > > > sys.platform with some additional manipulation like standardizing things > like > > > "linux3_x86", "linux2_x86", etc. to all be "linux_x86". I'm not sure if we > > make > > > similar replacements, but I know that using these interchangeably before has > > > given us problems, like in https://codereview.chromium.org/2608773002/, > which > > I > > > still need to submit. > > > > Waiting on neds feedback on this. > > Yeah. Better to use platform provided by callsites instead of inferring it > yourself. Remember that Telemetry supports running test on a remote platforms, > so the sys.platform value here is could be wrong if Telemetry is running on a > Linux host & control the test on Android. +Juan: can you take a pass at this too?
https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/120001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:16: # TODO(rnephew): Move this file to archive_info.py when it is the default. On 2017/01/18 19:30:42, nednguyen wrote: > On 2017/01/18 18:55:03, rnephew (Reviews Here) wrote: > > On 2017/01/18 16:07:58, charliea wrote: > > > Could you elaborate on why this is necessary and we can't just work these > > > changes into archive_info.py right now? I'm generally pretty against forking > > > code unless there's a very strong reason to do so. (Regardless, it probably > > > makes sense to document this in the code, too, as other people might wonder > > the > > > same thing) > > > > I said more about why. I could have had this be directly part of > archive_info.py > > but it would have been messier with a lot of if/else blocks and harder to > ensure > > that it is doing the correct thing. Since archive_info.py will be deleted > > relatively shortly (once I update all wpr.json files to the new format) I > > thought it would be easier to just split them and replace archive_info.py with > > archive_info2.py after the fact. > > I support this. In this case, it's cleaner to have: > > If this is the old json: > old_object = OldAPI(old_json) > old_object.DoFoo() > old_object.DoBar() > else: > new_object = NewAPI(new_json) > new_object.DoFoo() > new_object.DoBar() > > instead of: > > Class OldObject(): > .. > def DoFoo(): > if this is old: > Do old stuff > elif this is new: > Do new stuff > > def DoBar(): > if this is old: > Do old stuff > elif this is new: > Do new stuff SGTM https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info.py:62: if data.get('multiplatform', False): Should this also be platform_specific? https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:43: assert data.get('multiplatform', False), ( same note about multiplatform => platform_specific https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:98: current_platform = sys.platform problem with target_platform/sys.platform interchangeability still exists https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:1: # Copyright 2012 The Chromium Authors. All rights reserved. This is showing up as a completely new file rather than a copy + modify which is what it actually is. Because of this, it's pretty hard to figure out what the actual changes are. Could you make it so that this shows up as a modification instead? http://stackoverflow.com/questions/433111/how-to-make-git-mark-a-deleted-and-... suggests that the way to do this might be to do the copy and the modification of that copy in two separate commits.
https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info.py:62: if data.get('multiplatform', False): On 2017/01/18 22:35:51, charliea wrote: > Should this also be platform_specific? Done. https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:43: assert data.get('multiplatform', False), ( On 2017/01/18 22:35:51, charliea wrote: > same note about multiplatform => platform_specific Done. https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:98: current_platform = sys.platform On 2017/01/18 22:35:51, charliea wrote: > problem with target_platform/sys.platform interchangeability still exists What is the way the rest of telemetry got around this? BattOr uses sys.platform without problem. https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:1: # Copyright 2012 The Chromium Authors. All rights reserved. On 2017/01/18 22:35:51, charliea wrote: > This is showing up as a completely new file rather than a copy + modify which is > what it actually is. Because of this, it's pretty hard to figure out what the > actual changes are. Could you make it so that this shows up as a modification > instead? > > http://stackoverflow.com/questions/433111/how-to-make-git-mark-a-deleted-and-... > suggests that the way to do this might be to do the copy and the modification of > that copy in two separate commits. The original archive_info_unittest.py only had 6 tests, this has 15 tests. While some of them are similar, I think they need to be reviewed as new tests. I do not think the old ones were sufficient and the differences between these and the old ones are enough to warrant it.
The CQ bit was checked by rnephew@chromium.org 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: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by rnephew@chromium.org 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: This issue passed the CQ dry run.
On 2017/01/19 16:24:53, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > File telemetry/telemetry/wpr/archive_info.py (right): > > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > telemetry/telemetry/wpr/archive_info.py:62: if data.get('multiplatform', False): > On 2017/01/18 22:35:51, charliea wrote: > > Should this also be platform_specific? > > Done. > > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > File telemetry/telemetry/wpr/archive_info2.py (right): > > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > telemetry/telemetry/wpr/archive_info2.py:43: assert data.get('multiplatform', > False), ( > On 2017/01/18 22:35:51, charliea wrote: > > same note about multiplatform => platform_specific > > Done. > > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > telemetry/telemetry/wpr/archive_info2.py:98: current_platform = sys.platform > On 2017/01/18 22:35:51, charliea wrote: > > problem with target_platform/sys.platform interchangeability still exists > > What is the way the rest of telemetry got around this? BattOr uses sys.platform > without problem. > > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > File telemetry/telemetry/wpr/archive_info2_unittest.py (right): > > https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... > telemetry/telemetry/wpr/archive_info2_unittest.py:1: # Copyright 2012 The > Chromium Authors. All rights reserved. > On 2017/01/18 22:35:51, charliea wrote: > > This is showing up as a completely new file rather than a copy + modify which > is > > what it actually is. Because of this, it's pretty hard to figure out what the > > actual changes are. Could you make it so that this shows up as a modification > > instead? > > > > > http://stackoverflow.com/questions/433111/how-to-make-git-mark-a-deleted-and-... > > suggests that the way to do this might be to do the copy and the modification > of > > that copy in two separate commits. > > The original archive_info_unittest.py only had 6 tests, this has 15 tests. While > some of them are similar, I think they need to be reviewed as new tests. I do > not think the old ones were sufficient and the differences between these and the > old ones are enough to warrant it. Ping
https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:98: current_platform = sys.platform I'm not sure: I think we need to find some way to pipe it through. BattOr unit tests actually do have a problem with this: see https://codereview.chromium.org/2608773002/. We had to disable TracingControllerTest.testBattOrTracing on the Linux CQ because of confusion around sys.platform/target_platform https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/140001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:1: # Copyright 2012 The Chromium Authors. All rights reserved. On 2017/01/19 16:24:53, rnephew (Reviews Here) wrote: > On 2017/01/18 22:35:51, charliea wrote: > > This is showing up as a completely new file rather than a copy + modify which > is > > what it actually is. Because of this, it's pretty hard to figure out what the > > actual changes are. Could you make it so that this shows up as a modification > > instead? > > > > > http://stackoverflow.com/questions/433111/how-to-make-git-mark-a-deleted-and-... > > suggests that the way to do this might be to do the copy and the modification > of > > that copy in two separate commits. > > The original archive_info_unittest.py only had 6 tests, this has 15 tests. While > some of them are similar, I think they need to be reviewed as new tests. I do > not think the old ones were sufficient and the differences between these and the > old ones are enough to warrant it. Sounds good. I'll review them as new tests. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:44: 'Must be platform specific mode for this to work') Maybe be more specific here? I'd have no idea what this meant if I got this assertion and hadn't reviewed the code/design doc. Maybe something like: "archive_info2.WprArchiveInfo is exclusively for platform specific WPR archives. Platform agnostic archives should use archive_info.WprArchiveInfo." https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:1: # Copyright 2012 The Chromium Authors. All rights reserved. s/2012/2017 https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:10: from py_utils import cloud_storage # pylint: disable=import-error Any idea why this is an import-error? https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:56: class WprArchiveInfoTest2(unittest.TestCase): Maybe "WprArchiveInfo2Test" to match the file name? https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:81: old_ch = cloud_storage.CalculateHash Any reason we can't use @patch here, which automatically handles the function patching? Seems like we're doing it manually instead, which is more verbose and error-prone https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:91: def testInitNotMultiplatform(self): s/multiplatform/platformspecific https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:112: self.assertEquals(len(cloud_storage_stub.downloaded_files), 1) (here and elsewhere): any reason we can't use: self.assertItemsEqual(cloud_storage_stub.downloaded_files, [recording1]); to replace both the length and the member assert? https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:147: self.archive_info._bucket = None Accessing private members of the class that you're testing is generally considered a worst-practice. It almost always points to the idea that something has gone awry elsewhere. In this case, it seems like the thing that's gone awry is that the test class, WprArchiveInfoTest2, assumes that archive_info will only ever be constructed one way (using cloud_storage.PUBLIC_BUCKET), but that's not in fact true. It seems like the correct thing to do here is to have some sort of method like CreateArchiveInfo() with keyword parameters for each of the constructor parameters that the tester might want to play with. Then, in the tests, rather than using self.archive_info as the archive info, it would use archive_info = CreateArchiveInfo(cloud_storage_bucket=None) (at least in this test) The reason for this is that, the way that this is currently written assumes a *lot* of implementation details of archive info, which in turn means that someone browsing the unit tests to understand how archive info works has to already *have* all of that knowledge of archive_info before the unit tests even make sense. For example, what if archive_info._bucket is used in the constructor to initialize another private member, and by setting it here, we're accessing it too late in the object's lifecycle? Maybe that's what the original test author *meant* to do, and they understand the implications of that well enough to count on the ramifications? It's better to just bypass all of that and say "only access public members", and we know we can count on things working as expected in this case. For more info, see https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:151: def testDownloadArchivesIfneededBadJson(self): s/Ifneeded/IfNeeded https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:164: del self.archive_info._data['archives'] see previous note about not accessing private data https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:184: self.archive_info.WprFilePathForStory(page1, target_platform=mac), (here and in other platform-specific tests): my preference here would be to just hardcode the platform string into each of these calls: see: https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... "Inlining is often preferable for very simple values - the advantages include making its definition immediately available, reducing cognitive load by not requiring a reader to keep an additional variable in mind, and making a test read more like English by causing the object (a function argument) to follow the subject (an object on which a function is invoked)." https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:218: linux = 'linux2' I could be wrong, but I think 'linux' is the target_platform and 'linux2' is the sys.platform https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:233: def testAddRecordedStoriesNoStories(self): high-level critique: many of your tests follow the form test<Method><Circumstances>. Instead of doing this, it's better to test individual *behaviors*. Many times there are multiple behaviors associated with a single method, and testing this way allows people to add new or remove existing behaviors without changing tests for other behaviors. For more info, see https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... One good thing to keep in mind when writing unit tests is, to the greatest extent possible, you should make them "obviously correct". This test fails to meet that muster. Why do you copy the old data, add a temporary recording with no stories, and ensure that the new data meets that old data? What is even in _data? ("data" could mean just about anything). Why is the data that's currently in archive_info equivalent to a new temporary recording with no stories? Maybe this has to do with some of its constructor parameters? (This harkens back to the thing about constructing the archive_info within the test, not before it). There are just a lot of unanswered questions in such a small test that make it a lot less useful as a unit test. I'd definitely highly encourage you to read the unit testing best practices doc in its entirety: I can honestly say it's been one of the most useful things that I've learned at Google, and writing unit tests in the way that it describes is truly transformative. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:283: # Ensure the saved json does not contian trailing spaces. (here and elsewhere) s/json/JSON https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:283: # Ensure the saved json does not contian trailing spaces. (here and elsewhere) s/contian/contain
https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:44: 'Must be platform specific mode for this to work') On 2017/01/20 18:19:58, charliea wrote: > Maybe be more specific here? I'd have no idea what this meant if I got this > assertion and hadn't reviewed the code/design doc. Maybe something like: > > "archive_info2.WprArchiveInfo is exclusively for platform specific WPR archives. > Platform agnostic archives should use archive_info.WprArchiveInfo." Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:1: # Copyright 2012 The Chromium Authors. All rights reserved. On 2017/01/20 18:19:58, charliea wrote: > s/2012/2017 Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:10: from py_utils import cloud_storage # pylint: disable=import-error On 2017/01/20 18:19:58, charliea wrote: > Any idea why this is an import-error? This part was just c/p'd from the old tests. Do not know why it was disabled. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:56: class WprArchiveInfoTest2(unittest.TestCase): On 2017/01/20 18:19:58, charliea wrote: > Maybe "WprArchiveInfo2Test" to match the file name? Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:81: old_ch = cloud_storage.CalculateHash On 2017/01/20 18:19:58, charliea wrote: > Any reason we can't use @patch here, which automatically handles the function > patching? Seems like we're doing it manually instead, which is more verbose and > error-prone This was c/p'd from the old unittests. This actually isn't being used anywhere here now that I look at it, so deleting it. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:91: def testInitNotMultiplatform(self): On 2017/01/20 18:19:58, charliea wrote: > s/multiplatform/platformspecific Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:112: self.assertEquals(len(cloud_storage_stub.downloaded_files), 1) On 2017/01/20 18:19:58, charliea wrote: > (here and elsewhere): any reason we can't use: > > self.assertItemsEqual(cloud_storage_stub.downloaded_files, [recording1]); > > to replace both the length and the member assert? Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:147: self.archive_info._bucket = None On 2017/01/20 18:19:58, charliea wrote: > Accessing private members of the class that you're testing is generally > considered a worst-practice. It almost always points to the idea that something > has gone awry elsewhere. In this case, it seems like the thing that's gone awry > is that the test class, WprArchiveInfoTest2, assumes that archive_info will only > ever be constructed one way (using cloud_storage.PUBLIC_BUCKET), but that's not > in fact true. > > It seems like the correct thing to do here is to have some sort of method like > CreateArchiveInfo() with keyword parameters for each of the constructor > parameters that the tester might want to play with. Then, in the tests, rather > than using self.archive_info as the archive info, it would use > > archive_info = CreateArchiveInfo(cloud_storage_bucket=None) > > (at least in this test) > > The reason for this is that, the way that this is currently written assumes a > *lot* of implementation details of archive info, which in turn means that > someone browsing the unit tests to understand how archive info works has to > already *have* all of that knowledge of archive_info before the unit tests even > make sense. For example, what if archive_info._bucket is used in the constructor > to initialize another private member, and by setting it here, we're accessing it > too late in the object's lifecycle? Maybe that's what the original test author > *meant* to do, and they understand the implications of that well enough to count > on the ramifications? It's better to just bypass all of that and say "only > access public members", and we know we can count on things working as expected > in this case. > > For more info, see > > https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... > > https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:151: def testDownloadArchivesIfneededBadJson(self): On 2017/01/20 18:19:58, charliea wrote: > s/Ifneeded/IfNeeded Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:151: def testDownloadArchivesIfneededBadJson(self): Ended up deleting this test. You cannot initialize it with bad data so you cannot download with bad json. Test was testing something that is literally impossible. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:184: self.archive_info.WprFilePathForStory(page1, target_platform=mac), On 2017/01/20 18:19:58, charliea wrote: > (here and in other platform-specific tests): my preference here would be to just > hardcode the platform string into each of these calls: > > see: > https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b... > > "Inlining is often preferable for very simple values - the advantages include > making its definition immediately available, reducing cognitive load by not > requiring a reader to keep an additional variable in mind, and making a test > read more like English by causing the object (a function argument) to follow the > subject (an object on which a function is invoked)." Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:218: linux = 'linux2' On 2017/01/20 18:19:58, charliea wrote: > I could be wrong, but I think 'linux' is the target_platform and 'linux2' is the > sys.platform Its currently done so that target_platform and sys.platform are the same. Waiting on more input from Ned before making changes around this. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:233: def testAddRecordedStoriesNoStories(self): The current design of archive_info.py is that you cannot add more recordings before calling AddNewTemporaryRecording(). To make them interchangeable at this time I kept that convention. It would require more refactoring to get rid of it and I will probably talk to Ned about doing so later, but I believe it is out of the scope of this CL. The AddNewTemporaryRecording just does setup work for adding recordings later. I dont like that personally, but again I think changing it is for a seperate CL after we get rid of the old archive_info.py completely. With the above conversation, I personally believe that it does meet the requirement of obviousness for the most part. If you add an empty recording, you would expect the data not to change since there is no new data to add. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:283: # Ensure the saved json does not contian trailing spaces. On 2017/01/20 18:19:58, charliea wrote: > (here and elsewhere) s/json/JSON Done. https://codereview.chromium.org/2626373004/diff/180001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:283: # Ensure the saved json does not contian trailing spaces. On 2017/01/20 18:19:58, charliea wrote: > (here and elsewhere) s/contian/contain Done.
Added only a few small comments. This is looking generally good to me, so feel free to land when others are happy too. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:30: def __init__(self, file_path, data, bucket, target_platform="Default"): nit: add a module level constant to hold the default platform name. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:46: 'archive_info.WprArchiveInfo') Maybe I lost a bit of context. Is the intended end-state to have both an archive_info and archive_info2? That seems a bit confusing. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:77: ' field is needed' % self._file_path nit: switch to outer double quotes, so no need to escape the inner ones (looks a bit more readable). https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:100: archive_path = self._WprFileNameToPath(story_archives[story]['Default']) nit: use constant rather than 'Default' https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:105: download_if_needed(archive_path) why do we download both the 'Default' and the current_platform? https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:101: {cloud_storage.PUBLIC_BUCKET: {recording1: "dummyhash", nit: make the change between "dummyhash" and "dummyhash1" more explicit (e.g. "dummyhash1_old"?), otherwise it looks more like a typo. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:151: archive_info.WprFilePathForStory(page1, target_platform='Default'), nit: The more I look at it, should the string for the default platform be 'DEFAULT'? that way it would look more like a keyword with a special meaning. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:221: archive_info = self.createArchiveInfo() There seems to be a lot of code repetition on these final tests. Could it be possible to factor out the common parts into some private method?
https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:46: 'archive_info.WprArchiveInfo') On 2017/01/23 23:41:59, perezju wrote: > Maybe I lost a bit of context. Is the intended end-state to have both an > archive_info and archive_info2? That seems a bit confusing. No, archive_info2 will become archive_info once all .json data files are converted to the new format and the current archive_info will go away.
https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:46: 'archive_info.WprArchiveInfo') On 2017/01/23 23:50:30, rnephew (Reviews Here) wrote: > On 2017/01/23 23:41:59, perezju wrote: > > Maybe I lost a bit of context. Is the intended end-state to have both an > > archive_info and archive_info2? That seems a bit confusing. > > No, archive_info2 will become archive_info once all .json data files are > converted to the new format and the current archive_info will go away. Could you update the error message then? The way it's phrased suggests otherwise.
lgtm, but defer to Ned on ultimate approval (especially wrt sys.platform/target_platform) https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:59: class WprArchiveInfo2Test2(unittest.TestCase): Why not WprArchiveInfo2Test? (right now, you have two 2s in the name)
https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:30: def __init__(self, file_path, data, bucket, target_platform="Default"): On 2017/01/23 23:41:58, perezju wrote: > nit: add a module level constant to hold the default platform name. Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:46: 'archive_info.WprArchiveInfo') On 2017/01/23 23:55:14, perezju wrote: > On 2017/01/23 23:50:30, rnephew (Reviews Here) wrote: > > On 2017/01/23 23:41:59, perezju wrote: > > > Maybe I lost a bit of context. Is the intended end-state to have both an > > > archive_info and archive_info2? That seems a bit confusing. > > > > No, archive_info2 will become archive_info once all .json data files are > > converted to the new format and the current archive_info will go away. > > Could you update the error message then? The way it's phrased suggests > otherwise. Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:77: ' field is needed' % self._file_path On 2017/01/23 23:41:58, perezju wrote: > nit: switch to outer double quotes, so no need to escape the inner ones (looks a > bit more readable). Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:100: archive_path = self._WprFileNameToPath(story_archives[story]['Default']) On 2017/01/23 23:41:58, perezju wrote: > nit: use constant rather than 'Default' Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2.py:105: download_if_needed(archive_path) On 2017/01/23 23:41:58, perezju wrote: > why do we download both the 'Default' and the current_platform? Now only downloading current_platform if it is part of the archive, and default otherwise. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:59: class WprArchiveInfo2Test2(unittest.TestCase): On 2017/01/24 01:09:53, charliea wrote: > Why not WprArchiveInfo2Test? > > (right now, you have two 2s in the name) Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:101: {cloud_storage.PUBLIC_BUCKET: {recording1: "dummyhash", On 2017/01/23 23:41:59, perezju wrote: > nit: make the change between "dummyhash" and "dummyhash1" more explicit (e.g. > "dummyhash1_old"?), otherwise it looks more like a typo. Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:151: archive_info.WprFilePathForStory(page1, target_platform='Default'), On 2017/01/23 23:41:59, perezju wrote: > nit: The more I look at it, should the string for the default platform be > 'DEFAULT'? that way it would look more like a keyword with a special meaning. Done. https://codereview.chromium.org/2626373004/diff/200001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:221: archive_info = self.createArchiveInfo() On 2017/01/23 23:41:59, perezju wrote: > There seems to be a lot of code repetition on these final tests. Could it be > possible to factor out the common parts into some private method? Done.
https://codereview.chromium.org/2626373004/diff/220001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/220001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:33: default_archive_info_contents = (""" this one is getting pretty hard to read, rather than doing string interpolation, could you json.dumps({ ... some suitable dict ...}) instead?
https://codereview.chromium.org/2626373004/diff/220001/telemetry/telemetry/wp... File telemetry/telemetry/wpr/archive_info2_unittest.py (right): https://codereview.chromium.org/2626373004/diff/220001/telemetry/telemetry/wp... telemetry/telemetry/wpr/archive_info2_unittest.py:33: default_archive_info_contents = (""" On 2017/01/24 18:42:02, perezju wrote: > this one is getting pretty hard to read, rather than doing string interpolation, > could you json.dumps({ ... some suitable dict ...}) instead? Done.
lgtm when Ned is happy
The CQ bit was checked by rnephew@chromium.org 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: This issue passed the CQ dry run.
On 2017/01/24 20:03:29, perezju wrote: > lgtm when Ned is happy The more I think about the sys.platform way of determining the platform the more I think it wont work. Android makes it fall apart... It will return linux even though its for android.
On 2017/01/24 22:59:02, rnephew (Reviews Here) wrote: > On 2017/01/24 20:03:29, perezju wrote: > > lgtm when Ned is happy > > The more I think about the sys.platform way of determining the platform the more > I think it wont work. Android makes it fall apart... It will return linux even > though its for android. https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... seems like a good candidate to replace it with, but I cannot find a place where I can pull the platform.GetOsName and feed it to archive_info. Anyone have any suggestions?
On 2017/01/24 23:08:50, rnephew (Reviews Here) wrote: > On 2017/01/24 22:59:02, rnephew (Reviews Here) wrote: > > On 2017/01/24 20:03:29, perezju wrote: > > > lgtm when Ned is happy > > > > The more I think about the sys.platform way of determining the platform the > more > > I think it wont work. Android makes it fall apart... It will return linux even > > though its for android. > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > seems like a good candidate to replace it with, but I cannot find a place where > I can pull the platform.GetOsName and feed it to archive_info. Anyone have any > suggestions? Oh bummer. You probably will need to delay the creation of wpr_archive in story_runner until the platform object is available (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...).
On 2017/01/25 00:27:12, nednguyen wrote: > On 2017/01/24 23:08:50, rnephew (Reviews Here) wrote: > > On 2017/01/24 22:59:02, rnephew (Reviews Here) wrote: > > > On 2017/01/24 20:03:29, perezju wrote: > > > > lgtm when Ned is happy > > > > > > The more I think about the sys.platform way of determining the platform the > > more > > > I think it wont work. Android makes it fall apart... It will return linux > even > > > though its for android. > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > seems like a good candidate to replace it with, but I cannot find a place > where > > I can pull the platform.GetOsName and feed it to archive_info. Anyone have any > > suggestions? > > Oh bummer. You probably will need to delay the creation of wpr_archive in > story_runner until the platform object is available > (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...). An alternative is we add target_platform parameter to story_set's constructor & have its default value be "telemetry.core.platform.GetHostPlatform.GetOsName()". User can override it to "android" if needed. This may work because these days, we clearly separate Android benchmarks from desktop one. The good news is we only need to care about special casing the WPR's platform on a number of system health stories, and all of those already have annotation showing which platform they support.
On 2017/01/25 00:33:34, nednguyen wrote: > On 2017/01/25 00:27:12, nednguyen wrote: > > On 2017/01/24 23:08:50, rnephew (Reviews Here) wrote: > > > On 2017/01/24 22:59:02, rnephew (Reviews Here) wrote: > > > > On 2017/01/24 20:03:29, perezju wrote: > > > > > lgtm when Ned is happy > > > > > > > > The more I think about the sys.platform way of determining the platform > the > > > more > > > > I think it wont work. Android makes it fall apart... It will return linux > > even > > > > though its for android. > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > seems like a good candidate to replace it with, but I cannot find a place > > where > > > I can pull the platform.GetOsName and feed it to archive_info. Anyone have > any > > > suggestions? > > > > Oh bummer. You probably will need to delay the creation of wpr_archive in > > story_runner until the platform object is available > > > (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...). > > An alternative is we add target_platform parameter to story_set's constructor & > have its default value be "telemetry.core.platform.GetHostPlatform.GetOsName()". > User can override it to "android" if needed. This may work because these days, > we clearly separate Android benchmarks from desktop one. > > The good news is we only need to care about special casing the WPR's platform on > a number of system health stories, and all of those already have annotation > showing which platform they support. I think it would be possible to add a method to archive_info2.py and story_set.py that calls the method in archive_info2.py that is something like 'SetTargetPlatform(target_platform)' Maybe even make it a context manager so that it auto goes back to default after calling story_sets WprFilePathForStory method. It would be used here: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
On 2017/01/25 16:42:24, rnephew (Reviews Here) wrote: > On 2017/01/25 00:33:34, nednguyen wrote: > > On 2017/01/25 00:27:12, nednguyen wrote: > > > On 2017/01/24 23:08:50, rnephew (Reviews Here) wrote: > > > > On 2017/01/24 22:59:02, rnephew (Reviews Here) wrote: > > > > > On 2017/01/24 20:03:29, perezju wrote: > > > > > > lgtm when Ned is happy > > > > > > > > > > The more I think about the sys.platform way of determining the platform > > the > > > > more > > > > > I think it wont work. Android makes it fall apart... It will return > linux > > > even > > > > > though its for android. > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > seems like a good candidate to replace it with, but I cannot find a place > > > where > > > > I can pull the platform.GetOsName and feed it to archive_info. Anyone have > > any > > > > suggestions? > > > > > > Oh bummer. You probably will need to delay the creation of wpr_archive in > > > story_runner until the platform object is available > > > > > > (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...). > > > > An alternative is we add target_platform parameter to story_set's constructor > & > > have its default value be > "telemetry.core.platform.GetHostPlatform.GetOsName()". > > User can override it to "android" if needed. This may work because these days, > > we clearly separate Android benchmarks from desktop one. > > > > The good news is we only need to care about special casing the WPR's platform > on > > a number of system health stories, and all of those already have annotation > > showing which platform they support. > > I think it would be possible to add a method to archive_info2.py and > story_set.py that calls the method in archive_info2.py that is something like > 'SetTargetPlatform(target_platform)' Maybe even make it a context manager so > that it auto goes back to default after calling story_sets WprFilePathForStory > method. It would be used here: > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... I implemented my above solution. PTAL.
The CQ bit was checked by rnephew@chromium.org 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: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
On 2017/01/25 20:47:30, rnephew (Reviews Here) wrote: > On 2017/01/25 16:42:24, rnephew (Reviews Here) wrote: > > On 2017/01/25 00:33:34, nednguyen wrote: > > > On 2017/01/25 00:27:12, nednguyen wrote: > > > > On 2017/01/24 23:08:50, rnephew (Reviews Here) wrote: > > > > > On 2017/01/24 22:59:02, rnephew (Reviews Here) wrote: > > > > > > On 2017/01/24 20:03:29, perezju wrote: > > > > > > > lgtm when Ned is happy > > > > > > > > > > > > The more I think about the sys.platform way of determining the > platform > > > the > > > > > more > > > > > > I think it wont work. Android makes it fall apart... It will return > > linux > > > > even > > > > > > though its for android. > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > > > > seems like a good candidate to replace it with, but I cannot find a > place > > > > where > > > > > I can pull the platform.GetOsName and feed it to archive_info. Anyone > have > > > any > > > > > suggestions? > > > > > > > > Oh bummer. You probably will need to delay the creation of wpr_archive in > > > > story_runner until the platform object is available > > > > > > > > > > (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...). > > > > > > An alternative is we add target_platform parameter to story_set's > constructor > > & > > > have its default value be > > "telemetry.core.platform.GetHostPlatform.GetOsName()". > > > User can override it to "android" if needed. This may work because these > days, > > > we clearly separate Android benchmarks from desktop one. > > > > > > The good news is we only need to care about special casing the WPR's > platform > > on > > > a number of system health stories, and all of those already have annotation > > > showing which platform they support. > > > > I think it would be possible to add a method to archive_info2.py and > > story_set.py that calls the method in archive_info2.py that is something like > > 'SetTargetPlatform(target_platform)' Maybe even make it a context manager so > > that it auto goes back to default after calling story_sets WprFilePathForStory > > method. It would be used here: > > > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... > > I implemented my above solution. PTAL. Sorry for not commenting earlier, but I am advise not to mutate the object state likek that because they makes it harder to understand the code. In particular, when anyone see the code "wpr_achive.target_platform", they then need to ask themselve: "has target_platform been intialized at this point?" Answering that can sometimes be difficult if wpr_archive is passed a round in multiple different contexts. So I recommend 2 alternative designs: 1) Make target_platform part of WprArchiveInfo2's constructor. That must mean anytime you see an instance of such class, it already initialized with an appropriate target_platform. 2) Remove self._target_platform & only use it where it needs to be used. I see there are 3 places: DownloadArchivesIfNeeded WprFilePathForStory AddRecordedStories For DownloadArchivesIfNeeded, you can just makes it download all the wpr archive files in all the specified platforms. Not the most optimal but I doubt it affects the bot cycling time & makes plumbing the code easier. For WprFilePathForStory, this one is easy because it's called from places that already have handle to the platform object. For AddRecordedStories, you need to find way to plumb platform to https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... . This probably can be done by refactoring wpr_recorder a little bit so that this method is invoked inside RecorderPageTest instead (and you should have the handle to platform there)
(my vote is for the first option, because it seems to more closely align with other Telemetry code I've seen)
On 2017/01/25 21:18:16, charliea wrote: > (my vote is for the first option, because it seems to more closely align with > other Telemetry code I've seen) The first option isn't possible. The place where the archive_info object is initialized has no notion of 'platform'.
On 2017/01/25 21:21:54, rnephew (Reviews Here) wrote: > On 2017/01/25 21:18:16, charliea wrote: > > (my vote is for the first option, because it seems to more closely align with > > other Telemetry code I've seen) > > The first option isn't possible. The place where the archive_info object is > initialized has no notion of 'platform'. Patch with option 2 is now up.
Description was changed from ========== [WPR] Implement platform dependent recordings for WPR archives. BUG=catapult:#2814 ========== to ========== [WPR] Implement platform dependent recordings for WPR archives. BUG=chromium:618698,catapult:#2814 ==========
Ping.
I think we are now identify the main blocker for this patch, which is resolving the plumbing of platform for _UpdateAndCheckArchives method. From the high level view, _UpdateAndCheckArchives does 2 things: 1) Fetch the WPR archive for stories in the story_set 2) Does error reporting for stories that are missing WPR archive. THe reason why it's hard to plumb platform object to _UpdateAndCheckArchives is because it happens too early in the pipeline, when we haven't constructed the shared_page_state instance for each stories group yet. To address this, I suggest changing _UpdateAndCheckArchives so that: 1) If fetch the WPR archives for stories in the story_set on all platforms --> no need for being optimized about only fetch Linux WPR on linux or Mac WPR on Mac --> no need to plumb "platform" here. 2) Move the error reporting logic to later state. Maybe inside page_set.WprFilePathForStory(...). 1) & 2) should be done in a separate CL since it's a lot of code moving around. You will also need another CL to refactor catapult/telemetry/telemetry/record_wpr.py so that wpr_archive_info.AddRecordedStories(..) is invoked at a proper time which you have access to the |platform| instance. https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:210: test, finder_options.Copy(), story_set).platform.GetOSName() I suggest not to instantiate the shared_state_class just to get the platform object since it can have side effects (like the case of discovering the devices on Android, which involve making adb calls) https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:212: current_platform = None This seems to assume that there is one single platform among all the stories https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:399: filtered_stories, current_platform): This makes the assumption that all the stories have a same platform, which may not be true. https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/st... File telemetry/telemetry/story/story_set.py (right): https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/st... telemetry/telemetry/story/story_set.py:159: if story.platform_specific: Why do we need story to declare this? Why not just look up the corresponding WPR archive entry for that stories?
Step 1 is done, it now downloads all platforms. I will move the checking in a different CL. https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:210: test, finder_options.Copy(), story_set).platform.GetOSName() On 2017/01/30 17:31:23, nednguyen wrote: > I suggest not to instantiate the shared_state_class just to get the platform > object since it can have side effects (like the case of discovering the devices > on Android, which involve making adb calls) Done. https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:212: current_platform = None On 2017/01/30 17:31:23, nednguyen wrote: > This seems to assume that there is one single platform among all the stories It assumes that telemetry is only running on one platform, and that the story will have a WPR entry for one of either that platform or 'DEFAULT' Since we are downloading all platforms now though, this will go away. https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/in... telemetry/telemetry/internal/story_runner.py:399: filtered_stories, current_platform): On 2017/01/30 17:31:23, nednguyen wrote: > This makes the assumption that all the stories have a same platform, which may > not be true. See above comment. https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/st... File telemetry/telemetry/story/story_set.py (right): https://codereview.chromium.org/2626373004/diff/280001/telemetry/telemetry/st... telemetry/telemetry/story/story_set.py:159: if story.platform_specific: On 2017/01/30 17:31:23, nednguyen wrote: > Why do we need story to declare this? Why not just look up the corresponding WPR > archive entry for that stories? This is required for the archive_info1 and archive_info2 split. I will get rid of it after that happens. I have reordered and added a comment to make that more clear.
lgtm with nits Before proceeding with update the WPR archive json format in tools/perf/. I suggest you update the one used in telemetry unittests first so we can be sure that things are working fine end to end. https://codereview.chromium.org/2626373004/diff/300001/telemetry/telemetry/st... File telemetry/telemetry/story/story_set.py (right): https://codereview.chromium.org/2626373004/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/story_set.py:160: if not story.platform_specific: Oh, instead of this, can you just change the API of both to wpr_archive_info.WprFilePathForStory(story, target_platform=None) & have wpr_archive_info version 1 ignore the target_platform bit?
I'll update the json in another CL that I will send to your shortly. I'm going to land this with nothing using it at first. https://codereview.chromium.org/2626373004/diff/300001/telemetry/telemetry/st... File telemetry/telemetry/story/story_set.py (right): https://codereview.chromium.org/2626373004/diff/300001/telemetry/telemetry/st... telemetry/telemetry/story/story_set.py:160: if not story.platform_specific: On 2017/01/30 19:33:36, nednguyen wrote: > Oh, instead of this, can you just change the API of both to > wpr_archive_info.WprFilePathForStory(story, target_platform=None) & have > wpr_archive_info version 1 ignore the target_platform bit? Done.
The CQ bit was checked by rnephew@chromium.org 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...
https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... File telemetry/telemetry/story/story.py (right): https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... telemetry/telemetry/story/story.py:141: def platform_specific(self): Remove this?
https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... File telemetry/telemetry/story/story.py (right): https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... telemetry/telemetry/story/story.py:141: def platform_specific(self): On 2017/01/30 19:52:05, nednguyen wrote: > Remove this? This current cl does not contain the WPR Record logic (thats the next CL) which will have to use this to know if it should save it as a platform specific one, or just default. I can remove it from this CL if you want, but it will just come back in that one.
On 2017/01/30 19:54:55, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... > File telemetry/telemetry/story/story.py (right): > > https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... > telemetry/telemetry/story/story.py:141: def platform_specific(self): > On 2017/01/30 19:52:05, nednguyen wrote: > > Remove this? > > This current cl does not contain the WPR Record logic (thats the next CL) which > will have to use this to know if it should save it as a platform specific one, > or just default. I can remove it from this CL if you want, but it will just come > back in that one. Can you remove it for now? In the next CL, we can have more context about how it's used & decide whether it's needed.
https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... File telemetry/telemetry/story/story.py (right): https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... telemetry/telemetry/story/story.py:141: def platform_specific(self): On 2017/01/30 19:54:55, rnephew (Reviews Here) wrote: > On 2017/01/30 19:52:05, nednguyen wrote: > > Remove this? > > This current cl does not contain the WPR Record logic (thats the next CL) which > will have to use this to know if it should save it as a platform specific one, > or just default. I can remove it from this CL if you want, but it will just come > back in that one. Done.
On 2017/01/30 20:01:06, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... > File telemetry/telemetry/story/story.py (right): > > https://codereview.chromium.org/2626373004/diff/320001/telemetry/telemetry/st... > telemetry/telemetry/story/story.py:141: def platform_specific(self): > On 2017/01/30 19:54:55, rnephew (Reviews Here) wrote: > > On 2017/01/30 19:52:05, nednguyen wrote: > > > Remove this? > > > > This current cl does not contain the WPR Record logic (thats the next CL) > which > > will have to use this to know if it should save it as a platform specific one, > > or just default. I can remove it from this CL if you want, but it will just > come > > back in that one. > > Done. lgtm for land & make progresses
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org, perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2626373004/#ps360001 (title: "Rebase")
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": 360001, "attempt_start_ts": 1485808607143920,
"parent_rev": "7cb7bf223a3a395296d2f8f09773ce66ec584975", "commit_rev":
"7af1a1e70eb99a3162fc2a1d82d209468a887a60"}
Message was sent while issue was closed.
Description was changed from ========== [WPR] Implement platform dependent recordings for WPR archives. BUG=chromium:618698,catapult:#2814 ========== to ========== [WPR] Implement platform dependent recordings for WPR archives. BUG=chromium:618698,catapult:#2814 Review-Url: https://codereview.chromium.org/2626373004 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:360001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:360001) has been created in https://codereview.chromium.org/2664043002/ by rnephew@chromium.org. The reason for reverting is: Chromium side tests are failing, and this shouldn't impact them. Reverting to figure out why.. |
