Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
946473002:
Move base_dir to user_story_set, remove file_path. (Closed)
|
Created:
5 years, 10 months ago by aiolos (Not reviewing) Modified:
5 years, 10 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMove base_dir to user_story_set, remove file_path.
BUG=454531
Committed: https://crrev.com/65ff7d22f8986ab2832045af09160dc753e40c5f
Cr-Commit-Position: refs/heads/master@{#317632}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Still use file_path instead of name. #Patch Set 3 : add file_path unittest to user_story_set. #
Total comments: 4
Patch Set 4 : #
Messages
Total messages: 27 (6 generated)
aiolos@chromium.org changed reviewers: + chrishenry@google.com, dtu@chromium.org
aiolos@chromium.org changed reviewers: + nednguyen@google.com, sullivan@chromium.org
On 2015/02/20 02:23:00, aiolos wrote: Adding more reviewers since Chris is busy with other work.
Also, retrying on bots since one failure was a failed compile which also failed without the patch, and the other failure was because the device couldn't be found. https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:27: # TODO(aiolos): When migrating page_set's over to user_story_set's, make I could add that change to this review, but it's going to touch a large number of files. I was concerned that the important changes for this CL could be lost in the noise; and it would be easy to do when moving the page_sets to user_story_sets.
https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:28: # sure that we are passing a directory, not a file. The user_story_set version of this just asserts it's a directory, while this one use the containing directory if it's a file. What's the reason for the different behavior?
https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:28: # sure that we are passing a directory, not a file. On 2015/02/20 20:29:11, sullivan wrote: > The user_story_set version of this just asserts it's a directory, while this one > use the containing directory if it's a file. What's the reason for the different > behavior? I wanted to make it stronger so that we're forcing people to use valid directories instead of just random strings if they don't want to use our default. It seems more likely to help people only do intelligent things. (Which is why I had to change the page_unittest since we were just using a random string.)
https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:28: # sure that we are passing a directory, not a file. On 2015/02/20 20:33:21, aiolos wrote: > On 2015/02/20 20:29:11, sullivan wrote: > > The user_story_set version of this just asserts it's a directory, while this > one > > use the containing directory if it's a file. What's the reason for the > different > > behavior? > > I wanted to make it stronger so that we're forcing people to use valid > directories instead of just random strings if they don't want to use our > default. It seems more likely to help people only do intelligent things. (Which > is why I had to change the page_unittest since we were just using a random > string.) I wanted user_story_set to have the behavior we want moving forward (must specify a valid dir_path), and have page_set pass through the information we currently support (either file_path or dir_path) in the form we want. Also, when I change the current page_sets to user_story_sets, any file_paths that are actual files will be changed to pass the folder instead- that's the todo above. For the record, most of the page_sets seems to specify the directory any way.
lgtm
https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:27: # TODO(aiolos): When migrating page_set's over to user_story_set's, make On 2015/02/20 20:24:39, aiolos wrote: > I could add that change to this review, but it's going to touch a large number > of files. I was concerned that the important changes for this CL could be lost > in the noise; and it would be easy to do when moving the page_sets to > user_story_sets. +1. patch should have small scope and easy to understand :-) https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/un... File tools/telemetry/telemetry/unittest_util/page_set_smoke_test.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/un... tools/telemetry/telemetry/unittest_util/page_set_smoke_test.py:22: logging.warning('Skipping %s: no archive data file', page_set.name) I am not sure about whether this is better. Wouldn't it easier to find the page set that's failed by file path rather than classname? It's ok to kill the existing "file_path", which may or may not match the file's location, but can we do s.t like this for user_story_set: def file_path(self): return inspect.getfile(self.__class__).replace('.pyc', '.py') https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/us... File tools/telemetry/telemetry/user_story/user_story_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/us... tools/telemetry/telemetry/user_story/user_story_set.py:49: return self.__class__.__name__ Is this change intentional?
New patchsets have been uploaded after l-g-t-m from sullivan@chromium.org
https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/un... File tools/telemetry/telemetry/unittest_util/page_set_smoke_test.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/un... tools/telemetry/telemetry/unittest_util/page_set_smoke_test.py:22: logging.warning('Skipping %s: no archive data file', page_set.name) On 2015/02/20 20:58:09, nednguyen wrote: > I am not sure about whether this is better. Wouldn't it easier to find the page > set that's failed by file path rather than classname? > > It's ok to kill the existing "file_path", which may or may not match the file's > location, but can we do s.t like this for user_story_set: > > def file_path(self): > return inspect.getfile(self.__class__).replace('.pyc', '.py') I actually did that first, and just noticed that I left the inspect import in user_story_set. I changed my mind to use name because file_path has historically not actually been useful since it could just been the name of a directory. I don't mind changing it to always use the real file_path at all. https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/us... File tools/telemetry/telemetry/user_story/user_story_set.py (right): https://codereview.chromium.org/946473002/diff/1/tools/telemetry/telemetry/us... tools/telemetry/telemetry/user_story/user_story_set.py:49: return self.__class__.__name__ On 2015/02/20 20:58:10, nednguyen wrote: > Is this change intentional? Done.
lgtm. Can you add a simple unittest cover for user_story_set.file_path?
New patchsets have been uploaded after l-g-t-m from nednguyen@google.com
On 2015/02/20 22:16:31, nednguyen wrote: > lgtm. Can you add a simple unittest cover for user_story_set.file_path? done.
https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_set_unittest.py (right): https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_set_unittest.py:41: self.assertEqual(inspect.getfile(self.__class__).replace('pyc', 'py'), Can we change the left side to os.path.abspath(__file__) instead? Since we are testing the interface, we would want write test like assertEqual(5, add(2, 3)) instead of: assertEqual(2 + 3, add(2, 3))
https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_set_unittest.py (right): https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_set_unittest.py:41: self.assertEqual(inspect.getfile(self.__class__).replace('pyc', 'py'), On 2015/02/20 22:52:17, nednguyen wrote: > Can we change the left side to os.path.abspath(__file__) instead? > > Since we are testing the interface, we would want write test like > assertEqual(5, add(2, 3)) > instead of: > assertEqual(2 + 3, add(2, 3)) I can do that on this call if you want, but it would fail in the one below.
https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_set_unittest.py (right): https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_set_unittest.py:41: self.assertEqual(inspect.getfile(self.__class__).replace('pyc', 'py'), On 2015/02/20 22:57:57, aiolos wrote: > On 2015/02/20 22:52:17, nednguyen wrote: > > Can we change the left side to os.path.abspath(__file__) instead? > > > > Since we are testing the interface, we would want write test like > > assertEqual(5, add(2, 3)) > > instead of: > > assertEqual(2 + 3, add(2, 3)) > > I can do that on this call if you want, but it would fail in the one below. Probably no need to test the one below. :-)
https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/user_story/user_story_set_unittest.py (right): https://codereview.chromium.org/946473002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/user_story/user_story_set_unittest.py:41: self.assertEqual(inspect.getfile(self.__class__).replace('pyc', 'py'), On 2015/02/20 22:59:01, nednguyen wrote: > On 2015/02/20 22:57:57, aiolos wrote: > > On 2015/02/20 22:52:17, nednguyen wrote: > > > Can we change the left side to os.path.abspath(__file__) instead? > > > > > > Since we are testing the interface, we would want write test like > > > assertEqual(5, add(2, 3)) > > > instead of: > > > assertEqual(2 + 3, add(2, 3)) > > > > I can do that on this call if you want, but it would fail in the one below. > > Probably no need to test the one below. :-) Done.
lgtm
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/946473002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/65ff7d22f8986ab2832045af09160dc753e40c5f Cr-Commit-Position: refs/heads/master@{#317632}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/951593003/ by jam@chromium.org. The reason for reverting is: breaks on main waterfall, i.e. http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... AssertionError: 'E:\\b\\build\\slave\\Win_7_Tests_x64__1_\\build\\src\\tools\\telemetry\\telemetry\\user_story\\user_story_set_unittest.pyc' != 'E:\\b\\build\\slave\\Win_7_Tests_x64__1_\\build\\src\\tools\\telemetry\\telemetry\\user_story\\user_story_set_unittest.py' not sure why this fails on main waterfall but not trybots?.
Message was sent while issue was closed.
Patchset #5 (id:80001) has been deleted
Message was sent while issue was closed.
On 2015/02/23 22:17:39, jam wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/951593003/ by mailto:jam@chromium.org. > > The reason for reverting is: breaks on main waterfall, i.e. > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... > > AssertionError: > 'E:\\b\\build\\slave\\Win_7_Tests_x64__1_\\build\\src\\tools\\telemetry\\telemetry\\user_story\\user_story_set_unittest.pyc' > != > 'E:\\b\\build\\slave\\Win_7_Tests_x64__1_\\build\\src\\tools\\telemetry\\telemetry\\user_story\\user_story_set_unittest.py' > > > not sure why this fails on main waterfall but not trybots?. I was looking for a .py file, but the bots it was failing on was using a .pyc file. Fixed in https://codereview.chromium.org/954563002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
