[Telemetry] Introduce shared_user_story_state for real.

[Telemetry] Introduce shared_user_story_state for real.

Move page.page_runner --> user_story.user_story_runner.

BUG=418278
Committed: https://crrev.com/7f4660dcd4c4476502a61eb7375c4a941d938b9b
Cr-Commit-Position: refs/heads/master@{#305119}

[nednguyen, 2014-11-20 18:15:14]

Patchset #1 (id:1) has been deleted

[nednguyen, 2014-11-20 18:15:20]

Patchset #1 (id:20001) has been deleted

[nednguyen, 2014-11-20 18:15:25]

Patchset #1 (id:40001) has been deleted

[nednguyen, 2014-11-20 18:15:38]

nednguyen@google.com changed reviewers:
+ chrishenry@google.com, dtu@chromium.org, tonyg@chromium.org

[chrishenry, 2014-11-20 19:01:24]

This looks fairly straightforward. Is there any part of the code you want me to
look more closely?

Other than the comments below, this looks good. lgtm

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_runner_unittest.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner_unittest.py:87: # user_story_runner
or shared_page_state unittest that tests the same logic.
Maybe these should be integration test between user_story_runner and page-based
shared user story state (some of them may be ok, or better, as unit tests on
shared user story state, but I'd like to keep some integration test coverage
within telemetry itself).

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/__init__.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/__init__.py:48: 
nit: 1 blank line to match surrounding.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/shared_user_story_state.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:7: """A class
styled on unittest.TestCase for handling test setup & teardown.
The fact that the class (or methods below) are styled on unittest.TestCase isn't
the important aspect of the pydoc I think. You want to describe what the
class/methods do first, and then you can add notes afterwards saying that this
is styled after unittest.TestCase.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:11: options:
Missing doc?

Shouldn't this be under __init__ pydoc?

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:28: def
WillRunUserStory(self, user_story):
nit: Let's have WillRun/Run/DidRun next to each other.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:54: """ This
method is styled on unittest.TestCase.tearDownClass.
The naming of TearDown for something that means TearDownClass may be a bit
confusing. Consider naming it TearDownClass or something else altogether.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:174: if
(user_story.shared_user_story_state_class is not
Btw, it might be a nice syntax to use decorator to specify the user story state
class.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:178:
user_story_groups[-1].AddUserStory(user_story)
if you have (A and B are the user story state)
A A A B B A A

You're going to have grouping like this right?
[A A A] [B B] [A A]

Do you not want to do this instead?
[A A A A A] [B B]

If not, you probably want to add comment as to why you choose this way.

[nednguyen, 2014-11-20 21:08:27]

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_runner_unittest.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner_unittest.py:87: # user_story_runner
or shared_page_state unittest that tests the same logic.
On 2014/11/20 19:01:24, chrishenry wrote:
> Maybe these should be integration test between user_story_runner and
page-based
> shared user story state (some of them may be ok, or better, as unit tests on
> shared user story state, but I'd like to keep some integration test coverage
> within telemetry itself).

Acknowledged.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/__init__.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/__init__.py:48: 
On 2014/11/20 19:01:24, chrishenry wrote:
> nit: 1 blank line to match surrounding.

Done.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/shared_user_story_state.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:7: """A class
styled on unittest.TestCase for handling test setup & teardown.
On 2014/11/20 19:01:24, chrishenry wrote:
> The fact that the class (or methods below) are styled on unittest.TestCase
isn't
> the important aspect of the pydoc I think. You want to describe what the
> class/methods do first, and then you can add notes afterwards saying that this
> is styled after unittest.TestCase.

Done.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:11: options:
On 2014/11/20 19:01:24, chrishenry wrote:
> Missing doc?
> 
> Shouldn't this be under __init__ pydoc?

Done.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:28: def
WillRunUserStory(self, user_story):
On 2014/11/20 19:01:24, chrishenry wrote:
> nit: Let's have WillRun/Run/DidRun next to each other.

Done.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/shared_user_story_state.py:54: """ This
method is styled on unittest.TestCase.tearDownClass.
On 2014/11/20 19:01:24, chrishenry wrote:
> The naming of TearDown for something that means TearDownClass may be a bit
> confusing. Consider naming it TearDownClass or something else altogether.

Done.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:174: if
(user_story.shared_user_story_state_class is not
On 2014/11/20 19:01:24, chrishenry wrote:
> Btw, it might be a nice syntax to use decorator to specify the user story
state
> class.

Err, I would avoid innovating the syntax for now.

[nednguyen, 2014-11-20 21:08:32]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-11-20 21:10:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/733303003/80001

[commit-bot: I haz the power, 2014-11-20 21:17:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 21:17:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chrishenry, 2014-11-20 21:54:28]

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:174: if
(user_story.shared_user_story_state_class is not
On 2014/11/20 21:08:27, nednguyen wrote:
> On 2014/11/20 19:01:24, chrishenry wrote:
> > Btw, it might be a nice syntax to use decorator to specify the user story
> state
> > class.
> 
> Err, I would avoid innovating the syntax for now.

Yeah, it's a comment for future, not for this patch.

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:178:
user_story_groups[-1].AddUserStory(user_story)
On 2014/11/20 19:01:24, chrishenry wrote:
> if you have (A and B are the user story state)
> A A A B B A A
> 
> You're going to have grouping like this right?
> [A A A] [B B] [A A]
> 
> Do you not want to do this instead?
> [A A A A A] [B B]
> 
> If not, you probably want to add comment as to why you choose this way.

Ping?

[nednguyen, 2014-11-20 22:15:02]

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/733303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:178:
user_story_groups[-1].AddUserStory(user_story)
On 2014/11/20 21:54:27, chrishenry wrote:
> On 2014/11/20 19:01:24, chrishenry wrote:
> > if you have (A and B are the user story state)
> > A A A B B A A
> > 
> > You're going to have grouping like this right?
> > [A A A] [B B] [A A]
> > 
> > Do you not want to do this instead?
> > [A A A A A] [B B]
> > 
> > If not, you probably want to add comment as to why you choose this way.
> 
> Ping?

I added documentation. We will add some check in page_set_unittest to make sure
no one does anything funny with the ordering.

[nednguyen, 2014-11-20 23:05:04]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-11-20 23:06:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/733303003/100001

[commit-bot: I haz the power, 2014-11-21 00:27:18]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2014-11-21 00:28:06]

Patchset 3 (id:??) landed as
https://crrev.com/7f4660dcd4c4476502a61eb7375c4a941d938b9b
Cr-Commit-Position: refs/heads/master@{#305119}

[aiolos (Not reviewing), 2014-11-21 23:17:08]

Why do we still have a page_runner_unittest with no page_runner? All of those
tests should be in user_story_runner_unittest to avoid confusion. Someone could
accidentally delete the page_runner_unittest since there is no page_runner, or
there could easily be rot between the two files.

[nednguyen, 2014-11-21 23:21:00]

On 2014/11/21 23:17:08, aiolos wrote:
> Why do we still have a page_runner_unittest with no page_runner? All of those
> tests should be in user_story_runner_unittest to avoid confusion. Someone
could
> accidentally delete the page_runner_unittest since there is no page_runner, or
> there could easily be rot between the two files.

Subsequent patch here: https://codereview.chromium.org/745383002/

[nednguyen, 2014-11-22 04:21:35]

A revert of this CL (patchset #3 id:100001) has been created in
https://codereview.chromium.org/752883002/ by nednguyen@google.com.

The reason for reverting is: Speculative revert 
BUG=435775.