Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
748013003:
Better error messages on archive failures, and stop running with partial page sets in user_story_ru… (Closed)
|
Created:
6 years, 1 month ago by aiolos (Not reviewing) Modified:
6 years ago CC:
chromium-reviews, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBetter error messages on archive failures, and stop running with partial page sets in user_story_runner.
BUG=435063
Committed: https://crrev.com/c3ee1e91e79fe18145334127e0bf9f460f128a0c
Cr-Commit-Position: refs/heads/master@{#307074}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fail more gracefully with warnings instead of exceptions. #
Total comments: 15
Patch Set 3 : change test url. #Patch Set 4 : Return boolean instead of the list of pages. #
Total comments: 1
Patch Set 5 : Change of parameters and comment. #
Total comments: 5
Patch Set 6 : return instead of exit #
Messages
Total messages: 32 (4 generated)
aiolos@chromium.org changed reviewers: + dtu@chromium.org, nednguyen@google.com
Changes to user_story_runner.py to give better error messages and throw exceptions when we're missing page set archives. Do we need unittests for the added exceptions?
On 2014/11/22 00:33:02, aiolos wrote: > Changes to user_story_runner.py to give better error messages and throw > exceptions when we're missing page set archives. > > Do we need unittests for the added exceptions? Prefer yes, but up to you.
https://codereview.chromium.org/748013003/diff/1/tools/telemetry/telemetry/us... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/1/tools/telemetry/telemetry/us... tools/telemetry/telemetry/user_story/user_story_runner.py:295: raise Exception('No archive_data_file for page set.') nit: raise some other kind of Error, not a generic Exception.
On 2014/11/22 00:48:29, dtu wrote: > https://codereview.chromium.org/748013003/diff/1/tools/telemetry/telemetry/us... > File tools/telemetry/telemetry/user_story/user_story_runner.py (right): > > https://codereview.chromium.org/748013003/diff/1/tools/telemetry/telemetry/us... > tools/telemetry/telemetry/user_story/user_story_runner.py:295: raise > Exception('No archive_data_file for page set.') > nit: raise some other kind of Error, not a generic Exception. Hold on, I am doing a speculative revert of user_story_runner due to https://code.google.com/p/chromium/issues/detail?id=435775
On 2014/11/22 00:48:29, dtu wrote: > https://codereview.chromium.org/748013003/diff/1/tools/telemetry/telemetry/us... > File tools/telemetry/telemetry/user_story/user_story_runner.py (right): > > https://codereview.chromium.org/748013003/diff/1/tools/telemetry/telemetry/us... > tools/telemetry/telemetry/user_story/user_story_runner.py:295: raise > Exception('No archive_data_file for page set.') > nit: raise some other kind of Error, not a generic Exception. Hold on, I am doing a speculative revert of user_story_runner due to https://code.google.com/p/chromium/issues/detail?id=435775
ptal.
nednguyen@google.com changed reviewers: + slamm@chromium.org
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = page_set.PageSet() If this test use real page_set, it shouldn't be in user_story_unittest. Also please file a way to test this without touching the real network.
On 2014/12/03 18:27:44, nednguyen wrote: > https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): > > https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = > page_set.PageSet() > If this test use real page_set, it shouldn't be in user_story_unittest. Also > please file a way to test this without touching the real network. I'm not sure what you mean by a test that doesn't touch the real network. _CheckArchives is only ever called on page_sets, and doesn't pull anything from cloud_storage (that's done in _UpdateUserStoryArchivesIfChanged.) The reason I'm checking in the json/wpr/wpr.sha1 files is that _CheckArchives only looks at the page set and the file system. I think we need to have a unittest making sure we're returning the correct data from _CheckArchives. We only ever call that with page sets, and it doesn't make sense to test it on something else. Are you asking that this test be moved somewhere else? It is definitely a unittest, not an end-to-end one, which is why I didn't put it in page_run_end_to_end_tests.py.
On 2014/12/03 18:47:37, aiolos wrote: > On 2014/12/03 18:27:44, nednguyen wrote: > > > https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py > (right): > > > > > https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = > > page_set.PageSet() > > If this test use real page_set, it shouldn't be in user_story_unittest. Also > > please file a way to test this without touching the real network. > > I'm not sure what you mean by a test that doesn't touch the real network. > _CheckArchives is only ever called on page_sets, and doesn't pull anything from > cloud_storage (that's done in _UpdateUserStoryArchivesIfChanged.) The reason I'm > checking in the json/wpr/wpr.sha1 files is that _CheckArchives only looks at the > page set and the file system. > > I think we need to have a unittest making sure we're returning the correct data > from _CheckArchives. We only ever call that with page sets, and it doesn't make > sense to test it on something else. Are you asking that this test be moved > somewhere else? It is definitely a unittest, not an end-to-end one, which is why > I didn't put it in page_run_end_to_end_tests.py. I see. Can you please answer inline with the comment? It makes it easier to follow the discussion.
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:278: def _CheckArchives(page_set, pages): I think this _CheckArchives should take in (archive_data_file, wpr_archive_info, pages) instead of page_set to to make it clearer and easier to test. At first look, it's hard to see why this takes both page_set & pages since page_set does contain pages list. https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:327: # Only run valid pages if no problems with the page set or individual pages. Base on this, I think this _Check... can just raise Exception to exist the test run instead. There is no need to return anything. https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:350: ps.AddPageWithDefaultRunNavigate('http://127.0.0.1:62350/blank.html') Is there any reason why you choose 'http://127.0.0.1:62350/blank.html' instead of a bogus url? If not, please change the url so it easier to reason about the testing logic.
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:278: def _CheckArchives(page_set, pages): On 2014/12/03 19:35:59, nednguyen wrote: > I think this _CheckArchives should take in (archive_data_file, wpr_archive_info, > pages) instead of page_set to to make it clearer and easier to test. At first > look, it's hard to see why this takes both page_set & pages since page_set does > contain pages list. Hmm. Good point. We don't actually need to return the correct set of pages here anymore. Pagesets won't always have wpr_archive_info attributes though. And it makes no sense to pull the check for that out of _CheckArchives (if we don't have an archive_data_file, we won't have a wpr_archive_info, that's why the checks are in that order.) If we want to remove the redundancy, we should just pass the page_set in, and maybe move the _ShuffleAndFilterUserStorySet after we check the page_set in Run() since we might not need to do that work. https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:327: # Only run valid pages if no problems with the page set or individual pages. On 2014/12/03 19:35:59, nednguyen wrote: > Base on this, I think this _Check... can just raise Exception to exist the test > run instead. There is no need to return anything. The output was clearer that it wasn't a failure in the user_story_runner script after I removed the exceptions from the first patch set. Do you want me to make new errors for these cases, put them back in the places I removed tthe exceptions from, and then put the call to _CheckArchives inside a try in Run() that exits more gracefully? https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = page_set.PageSet() On 2014/12/03 18:27:43, nednguyen wrote: > If this test use real page_set, it shouldn't be in user_story_unittest. Also > please file a way to test this without touching the real network. Is this something I should be considering still? I didn't see a response to it. https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:350: ps.AddPageWithDefaultRunNavigate('http://127.0.0.1:62350/blank.html') On 2014/12/03 19:35:59, nednguyen wrote: > Is there any reason why you choose 'http://127.0.0.1:62350/blank.html' instead > of a bogus url? If not, please change the url so it easier to reason about the > testing logic. I pulled it from an existing test page set. I don't think it matters if we use this or not. Done.
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:278: def _CheckArchives(page_set, pages): On 2014/12/04 00:29:31, aiolos wrote: > On 2014/12/03 19:35:59, nednguyen wrote: > > I think this _CheckArchives should take in (archive_data_file, > wpr_archive_info, > > pages) instead of page_set to to make it clearer and easier to test. At first > > look, it's hard to see why this takes both page_set & pages since page_set > does > > contain pages list. > > Hmm. Good point. We don't actually need to return the correct set of pages here > anymore. Pagesets won't always have wpr_archive_info attributes though. Are you sure about this? https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > And it > makes no sense to pull the check for that out of _CheckArchives (if we don't > have an archive_data_file, we won't have a wpr_archive_info, that's why the > checks are in that order.) If we want to remove the redundancy, we should just > pass the page_set in, and maybe move the _ShuffleAndFilterUserStorySet after we > check the page_set in Run() since we might not need to do that work. https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:327: # Only run valid pages if no problems with the page set or individual pages. On 2014/12/04 00:29:31, aiolos wrote: > On 2014/12/03 19:35:59, nednguyen wrote: > > Base on this, I think this _Check... can just raise Exception to exist the > test > > run instead. There is no need to return anything. > > The output was clearer that it wasn't a failure in the user_story_runner script > after I removed the exceptions from the first patch set. Do you want me to make > new errors for these cases, put them back in the places I removed tthe > exceptions from, and then put the call to _CheckArchives inside a try in Run() > that exits more gracefully? If you want this to exits gracefully, you can make this return True or False, which is essentially what the caller wants anyway (empty pages = True vs [] is False). In Run(), if this is not True, then exits early. https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = page_set.PageSet() On 2014/12/04 00:29:31, aiolos wrote: > On 2014/12/03 18:27:43, nednguyen wrote: > > If this test use real page_set, it shouldn't be in user_story_unittest. Also > > please file a way to test this without touching the real network. > > Is this something I should be considering still? I didn't see a response to it. Yes, my bad. Please ignore the original comment.
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:278: def _CheckArchives(page_set, pages): On 2014/12/04 05:17:31, nednguyen wrote: > On 2014/12/04 00:29:31, aiolos wrote: > > On 2014/12/03 19:35:59, nednguyen wrote: > > > I think this _CheckArchives should take in (archive_data_file, > > wpr_archive_info, > > > pages) instead of page_set to to make it clearer and easier to test. At > first > > > look, it's hard to see why this takes both page_set & pages since page_set > > does > > > contain pages list. > > > > Hmm. Good point. We don't actually need to return the correct set of pages > here > > anymore. Pagesets won't always have wpr_archive_info attributes though. > Are you sure about this? > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > Hmm. I'd made the change earlier, and had one of the tests fail on me. Perhaps I just mistyped it and didn't notice. Is it still better to pass in three arguments instead of one here? https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:327: # Only run valid pages if no problems with the page set or individual pages. On 2014/12/04 05:17:31, nednguyen wrote: > On 2014/12/04 00:29:31, aiolos wrote: > > On 2014/12/03 19:35:59, nednguyen wrote: > > > Base on this, I think this _Check... can just raise Exception to exist the > > test > > > run instead. There is no need to return anything. > > > > The output was clearer that it wasn't a failure in the user_story_runner > script > > after I removed the exceptions from the first patch set. Do you want me to > make > > new errors for these cases, put them back in the places I removed tthe > > exceptions from, and then put the call to _CheckArchives inside a try in Run() > > that exits more gracefully? > > If you want this to exits gracefully, you can make this return True or False, > which is essentially what the caller wants anyway (empty pages = True vs [] is > False). In Run(), if this is not True, then exits early. Done.
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:278: def _CheckArchives(page_set, pages): On 2014/12/04 19:23:31, aiolos wrote: > On 2014/12/04 05:17:31, nednguyen wrote: > > On 2014/12/04 00:29:31, aiolos wrote: > > > On 2014/12/03 19:35:59, nednguyen wrote: > > > > I think this _CheckArchives should take in (archive_data_file, > > > wpr_archive_info, > > > > pages) instead of page_set to to make it clearer and easier to test. At > > first > > > > look, it's hard to see why this takes both page_set & pages since > page_set > > > does > > > > contain pages list. > > > > > > Hmm. Good point. We don't actually need to return the correct set of pages > > here > > > anymore. Pagesets won't always have wpr_archive_info attributes though. > > Are you sure about this? > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > Hmm. I'd made the change earlier, and had one of the tests fail on me. Perhaps I > just mistyped it and didn't notice. Is it still better to pass in three > arguments instead of one here? Yes, please. https://codereview.chromium.org/748013003/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:282: Logs warnings if any are missing and returns an empty list. Update documentation.
https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:278: def _CheckArchives(page_set, pages): On 2014/12/04 19:38:09, nednguyen wrote: > On 2014/12/04 19:23:31, aiolos wrote: > > On 2014/12/04 05:17:31, nednguyen wrote: > > > On 2014/12/04 00:29:31, aiolos wrote: > > > > On 2014/12/03 19:35:59, nednguyen wrote: > > > > > I think this _CheckArchives should take in (archive_data_file, > > > > wpr_archive_info, > > > > > pages) instead of page_set to to make it clearer and easier to test. At > > > first > > > > > look, it's hard to see why this takes both page_set & pages since > > page_set > > > > does > > > > > contain pages list. > > > > > > > > Hmm. Good point. We don't actually need to return the correct set of pages > > > here > > > > anymore. Pagesets won't always have wpr_archive_info attributes though. > > > Are you sure about this? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > Hmm. I'd made the change earlier, and had one of the tests fail on me. Perhaps > I > > just mistyped it and didn't notice. Is it still better to pass in three > > arguments instead of one here? > > Yes, please. Can I ask why it's better to pass in three attributes of an object instead of the object itself here? I've had other reviewers tell me the exact opposite before (citing increased readability and/or reducing code size,) and I'd like to understand your reasoning. Do user stories have similar archives to check that aren't stored the same way? Or is there something else I'm missing here?
On 2014/12/04 20:13:38, aiolos wrote: > https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/user_story/user_story_runner.py (right): > > https://codereview.chromium.org/748013003/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/user_story/user_story_runner.py:278: def > _CheckArchives(page_set, pages): > On 2014/12/04 19:38:09, nednguyen wrote: > > On 2014/12/04 19:23:31, aiolos wrote: > > > On 2014/12/04 05:17:31, nednguyen wrote: > > > > On 2014/12/04 00:29:31, aiolos wrote: > > > > > On 2014/12/03 19:35:59, nednguyen wrote: > > > > > > I think this _CheckArchives should take in (archive_data_file, > > > > > wpr_archive_info, > > > > > > pages) instead of page_set to to make it clearer and easier to test. > At > > > > first > > > > > > look, it's hard to see why this takes both page_set & pages since > > > page_set > > > > > does > > > > > > contain pages list. > > > > > > > > > > Hmm. Good point. We don't actually need to return the correct set of > pages > > > > here > > > > > anymore. Pagesets won't always have wpr_archive_info attributes though. > > > > Are you sure about this? > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > > > > > Hmm. I'd made the change earlier, and had one of the tests fail on me. > Perhaps > > I > > > just mistyped it and didn't notice. Is it still better to pass in three > > > arguments instead of one here? > > > > Yes, please. > > > Can I ask why it's better to pass in three attributes of an object instead of > the object itself here? I've had other reviewers tell me the exact opposite > before (citing increased readability and/or reducing code size,) and I'd like to > understand your reasoning. Do user stories have similar archives to check that > aren't stored the same way? Or is there something else I'm missing here? You pass the objects if the semantic of the function is meant to deal with the object. In this case, it doesn't. By passing in the attributes of page_set: 1) You make it easier to unittest. You don't need to create page_set to test the methods. 2) If you ever want to move these attributes to different places, i.e: benchmark instead of page_set, you don't have to change the code _CheckArchives & your corresponding unittest 3) You make sure that there is less chance that _CheckArchives will any harm to the page_set instance. page_set is supposed to be immutable, but right now it's not.
ptal
https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:207: sys.exit() Can you just return here instead of using sys.exit()? https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = page_set.PageSet() There is no need to create page set instance to test.
https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner.py (right): https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner.py:207: sys.exit() On 2014/12/05 15:57:50, nednguyen wrote: > Can you just return here instead of using sys.exit()? Sure. https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = page_set.PageSet() On 2014/12/05 15:57:50, nednguyen wrote: > There is no need to create page set instance to test. That's not true. _CheckArchives checks whether the pages passed in have an archive_path, which calls into the page's pageset. So without the page set, we would always return False. https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
nednguyen@google.com changed reviewers: + chrishenry@google.com
LGTM +slamm, +chrishenry: we should make archive_path page attribute instead of page_set also. https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right): https://codereview.chromium.org/748013003/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:347: ps = page_set.PageSet() On 2014/12/05 17:20:11, aiolos wrote: > On 2014/12/05 15:57:50, nednguyen wrote: > > There is no need to create page set instance to test. > > That's not true. _CheckArchives checks whether the pages passed in have an > archive_path, which calls into the page's pageset. So without the page set, we > would always return False. > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... I see, that's funky.
On 2014/12/05 19:10:42, nednguyen wrote: > LGTM > > +slamm, +chrishenry: we should make archive_path page attribute instead of > page_set also. > The archive_path is actually stored in a dictionary (page, archive path) in the pageset's wpr_archive_info. That path is updated each time we record the pageset with record_wpr. You would either need to couple the page_set_archive_info to the pages in it's pageset to have it update each of them, or have the page_set_archive_info be an attribute in the page. I'm not sure that either of those is a good idea.
On 2014/12/05 19:25:24, aiolos wrote: > On 2014/12/05 19:10:42, nednguyen wrote: > > LGTM > > > > +slamm, +chrishenry: we should make archive_path page attribute instead of > > page_set also. > > > > The archive_path is actually stored in a dictionary (page, archive path) in the > pageset's wpr_archive_info. That path is updated each time we record the pageset > with record_wpr. You would either need to couple the page_set_archive_info to > the pages in it's pageset to have it update each of them, or have the > page_set_archive_info be an attribute in the page. I'm not sure that either of > those is a good idea. Implementation asides, decouple the page from page_set is a direction we are aiming for telemetry. Essentially, page_set (user_story_set) should just be a list.
On 2014/12/05 19:31:36, nednguyen wrote: > On 2014/12/05 19:25:24, aiolos wrote: > > On 2014/12/05 19:10:42, nednguyen wrote: > > > LGTM > > > > > > +slamm, +chrishenry: we should make archive_path page attribute instead of > > > page_set also. > > > > > > > The archive_path is actually stored in a dictionary (page, archive path) in > the > > pageset's wpr_archive_info. That path is updated each time we record the > pageset > > with record_wpr. You would either need to couple the page_set_archive_info to > > the pages in it's pageset to have it update each of them, or have the > > page_set_archive_info be an attribute in the page. I'm not sure that either of > > those is a good idea. > > Implementation asides, decouple the page from page_set is a direction we are > aiming for telemetry. Essentially, page_set (user_story_set) should just be a > list. Another way would be to promote the WPR archive info to user_story_set. Seems cleaner given that WPR is a concept independent from a page (like network is independent from browser -- hence network_controller in platform).
On 2014/12/05 19:31:36, nednguyen wrote: > On 2014/12/05 19:25:24, aiolos wrote: > > On 2014/12/05 19:10:42, nednguyen wrote: > > > LGTM > > > > > > +slamm, +chrishenry: we should make archive_path page attribute instead of > > > page_set also. > > > > > > > The archive_path is actually stored in a dictionary (page, archive path) in > the > > pageset's wpr_archive_info. That path is updated each time we record the > pageset > > with record_wpr. You would either need to couple the page_set_archive_info to > > the pages in it's pageset to have it update each of them, or have the > > page_set_archive_info be an attribute in the page. I'm not sure that either of > > those is a good idea. > > Implementation asides, decouple the page from page_set is a direction we are > aiming for telemetry. Essentially, page_set (user_story_set) should just be a > list. We should re-think how we're recording and storing the archives then. If pagesets are just lists, we probably shouldn't have page_set_archive_info's at all.
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/748013003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c3ee1e91e79fe18145334127e0bf9f460f128a0c Cr-Commit-Position: refs/heads/master@{#307074} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
