[pinpoint] Add QuestGenerator object.

[pinpoint] Add QuestGenerator object.

Telemetry and GTest tests have a few significant differences between them. The
new QuestGenerators abstract out the differences between different types of
tests. They have the ability to read different request parameters for different
test types and customize the list of Quests for each type.

BUG=catapult:#3729

Review-Url: https://codereview.chromium.org/2996473002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/cd8447f51123eb08223731470b6f10f150b81142

[dtu, 2017-08-03 23:23:04]

dtu@chromium.org changed reviewers:
+ perezju@chromium.org, simonhatch@chromium.org, sullivan@chromium.org

[perezju, 2017-08-07 12:06:12]

lgtm, with some minor comments and nits

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/handlers/new.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/handlers/new.py:34: test_config =
self.request.get('test_config')
Seems like this is not needed? (Value is thrown away in line 53).

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/job.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:64: test_config =
ndb.PickleProperty()
nit: wondering whether this should be called target_config??

also wondering whether the target string should be part of the "config" object?

Not sure about either of these, but what are your thoughts?

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/quest/read_value.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/quest/read_value.py:19: return
(isinstance(other, self.__class__) and
nit: prefer type(self) over self.__class__

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/quest/run_test.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/quest/run_test.py:59: return
(isinstance(other, self.__class__) and
nit: ditto

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/quest/run_test.py:66: return 'Test'
Is there anything more useful we could return?

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/test_config.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/test_config.py:65: def _arguments(self):
nit: not a strong opinion, but I would lean towards making this a regular method
and not a property (as it builds and returns something mutable).

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/test_config.py:81: ]
Factor out these common args to a module level (private) constant?

[dtu, 2017-08-15 17:06:41]

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/handlers/new.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/handlers/new.py:34: test_config =
self.request.get('test_config')
On 2017/08/07 12:06:12, perezju wrote:
> Seems like this is not needed? (Value is thrown away in line 53).

Done. Previously I was going to put all the test config parameters as subfields
of this field, but ended up putting them all at the top level.

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/job.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:64: test_config =
ndb.PickleProperty()
On 2017/08/07 12:06:12, perezju wrote:
> nit: wondering whether this should be called target_config??
> 
> also wondering whether the target string should be part of the "config"
object?
> 
> Not sure about either of these, but what are your thoughts?

`target` has to be "config" object because it needs to be passed into
FindIsolate().

But then we could also move FindIsolate inside Config.Quests(), making Config
the sole source for the Quest list. Then instead of a "config" object, it's some
kind of QuestGenerator object. That sounds pretty elegant.

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/quest/read_value.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/quest/read_value.py:19: return
(isinstance(other, self.__class__) and
On 2017/08/07 12:06:12, perezju wrote:
> nit: prefer type(self) over self.__class__

Done.

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/quest/run_test.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/quest/run_test.py:59: return
(isinstance(other, self.__class__) and
On 2017/08/07 12:06:12, perezju wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/quest/run_test.py:66: return 'Test'
On 2017/08/07 12:06:12, perezju wrote:
> Is there anything more useful we could return?

Not sure if we need more with the current UI. We want to be able to distinguish
between the "Build" and "Test" results, but the config details are already
available at the top-level through TestConfig.AsDict().

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/test_config.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/test_config.py:65: def _arguments(self):
On 2017/08/07 12:06:12, perezju wrote:
> nit: not a strong opinion, but I would lean towards making this a regular
method
> and not a property (as it builds and returns something mutable).

Done.

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/test_config.py:81: ]
On 2017/08/07 12:06:12, perezju wrote:
> Factor out these common args to a module level (private) constant?

It will make more sense in the follow-up :)

[dtu, 2017-08-15 18:01:39]

Description was changed from

==========
[pinpoint] Add TestConfig object.

Telemetry and GTest tests have a few significant differences between them. The
new TestConfigs abstract out the differences between different types of tests.
They have the ability to read different request parameters for different test
types and customize the list of Quests for each type.

BUG=catapult:#3729
==========

to

==========
[pinpoint] Add QuestGenerator object.

Telemetry and GTest tests have a few significant differences between them. The
new QuestGenerators abstract out the differences between different types of
tests.  They have the ability to read different request parameters for different
test types and customize the list of Quests for each type.

BUG=catapult:#3729
==========

[dtu, 2017-08-15 18:01:48]

Description was changed from

==========
[pinpoint] Add QuestGenerator object.

Telemetry and GTest tests have a few significant differences between them. The
new QuestGenerators abstract out the differences between different types of
tests.  They have the ability to read different request parameters for different
test types and customize the list of Quests for each type.

BUG=catapult:#3729
==========

to

==========
[pinpoint] Add QuestGenerator object.

Telemetry and GTest tests have a few significant differences between them. The
new QuestGenerators abstract out the differences between different types of
tests. They have the ability to read different request parameters for different
test types and customize the list of Quests for each type.

BUG=catapult:#3729
==========

[perezju, 2017-08-16 08:15:26]

lgtm w/nits

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/job.py (right):

https://codereview.chromium.org/2996473002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:64: test_config =
ndb.PickleProperty()
On 2017/08/15 17:06:41, dtu wrote:
> On 2017/08/07 12:06:12, perezju wrote:
> > nit: wondering whether this should be called target_config??
> > 
> > also wondering whether the target string should be part of the "config"
> object?
> > 
> > Not sure about either of these, but what are your thoughts?
> 
> `target` has to be "config" object because it needs to be passed into
> FindIsolate().
> 
> But then we could also move FindIsolate inside Config.Quests(), making Config
> the sole source for the Quest list. Then instead of a "config" object, it's
some
> kind of QuestGenerator object. That sounds pretty elegant.

Nice! This turned out pretty neat!

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/jobs_test.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/jobs_test.py:41: 'target':
'telemetry_perf_tests',
Use plain the plain QuestGenerator here (rather than TelemetryQuestGenerator),
which should use the target value to figure out the correct generator tu use.

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/new_test.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/new_test.py:114: self.assertIn('error',
json.loads(response.body))
nit: also add test for missing target

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/models/quest_generator.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/models/quest_generator.py:97:
arguments.append('--browser=' + self.browser)
nit: arguments += ..

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/models/quest_generator_test.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/models/quest_generator_test.py:17:
'${ISOLATED_OUTDIR}/chartjson-output.json',
super nit: It's a bit awkward to mix '--flag=value' and ('--flag', 'value')
args; I would vote to make them all like the later.

[dtu, 2017-08-16 17:31:48]

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/jobs_test.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/jobs_test.py:41: 'target':
'telemetry_perf_tests',
On 2017/08/16 08:15:25, perezju wrote:
> Use plain the plain QuestGenerator here (rather than TelemetryQuestGenerator),
> which should use the target value to figure out the correct generator tu use.

Done.

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/handlers/new_test.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/handlers/new_test.py:114: self.assertIn('error',
json.loads(response.body))
On 2017/08/16 08:15:25, perezju wrote:
> nit: also add test for missing target

Done.

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/models/quest_generator.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/models/quest_generator.py:97:
arguments.append('--browser=' + self.browser)
On 2017/08/16 08:15:26, perezju wrote:
> nit: arguments += ..

Done.

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
File dashboard/dashboard/pinpoint/models/quest_generator_test.py (right):

https://codereview.chromium.org/2996473002/diff/20001/dashboard/dashboard/pin...
dashboard/dashboard/pinpoint/models/quest_generator_test.py:17:
'${ISOLATED_OUTDIR}/chartjson-output.json',
On 2017/08/16 08:15:26, perezju wrote:
> super nit: It's a bit awkward to mix '--flag=value' and ('--flag', 'value')
> args; I would vote to make them all like the later.

Done.

[dtu, 2017-08-16 17:31:54]

The CQ bit was checked by dtu@chromium.org

[dtu, 2017-08-16 17:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/2996473002/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2017-08-16 17:32:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-16 17:58:32]

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

[commit-bot: I haz the power, 2017-08-16 17:58:33]

Failed to apply patch for
dashboard/dashboard/pinpoint/models/quest/run_test_test.py:
While running git apply --index -3 -p1;
  error: patch failed:
dashboard/dashboard/pinpoint/models/quest/run_test_test.py:122
  Falling back to three-way merge...
  Applied patch to 'dashboard/dashboard/pinpoint/models/quest/run_test_test.py'
with conflicts.
  U dashboard/dashboard/pinpoint/models/quest/run_test_test.py

Patch:       dashboard/dashboard/pinpoint/models/quest/run_test_test.py
Index: dashboard/dashboard/pinpoint/models/quest/run_test_test.py
diff --git a/dashboard/dashboard/pinpoint/models/quest/run_test_test.py
b/dashboard/dashboard/pinpoint/models/quest/run_test_test.py
index
08497822807b535caaf38550c8c89d6c61f3b1a1..f6a344e63d73e78428cc61e507b789513ee440cc
100644
--- a/dashboard/dashboard/pinpoint/models/quest/run_test_test.py
+++ b/dashboard/dashboard/pinpoint/models/quest/run_test_test.py
@@ -9,8 +9,8 @@ import mock
 from dashboard.pinpoint.models.quest import run_test
 
 
-_SWARMING_TASK_EXTRA_ARGS = [
-    'test_suite', '--story-filter', 'test',
+_SWARMING_EXTRA_ARGS = [
+    'benchmark', '--story-filter', 'story',
     '-v', '--upload-results',
     '--output-format=chartjson', '--browser=release',
     '--isolated-script-test-output=${ISOLATED_OUTDIR}/output.json',
@@ -18,42 +18,41 @@ _SWARMING_TASK_EXTRA_ARGS = [
     '${ISOLATED_OUTDIR}/chartjson-output.json',
 ]
 
+_SWARMING_DIMENSIONS = [
+    {"key": "cores", "value": "8"},
+    {"key": "gpu", "value": "1002:6821"},
+    {"key": "os", "value": "Mac-10.11"},
+]
+
 
 class _RunTestTest(unittest.TestCase):
 
   def assertNewTaskHasDimensions(self, swarming_tasks_new):
     body = {
-        'name': 'Pinpoint job on chromium-rel-mac11-pro',
+        'name': 'Pinpoint job',
         'user': 'Pinpoint',
         'priority': '100',
         'expiration_secs': '600',
         'properties': {
             'inputs_ref': {'isolated': 'input isolate hash'},
-            'extra_args': _SWARMING_TASK_EXTRA_ARGS,
-            'dimensions': [
-                {'key': 'pool', 'value': 'Chrome-perf-pinpoint'},
-                {"key": "cores", "value": "8"},
-                {"key": "gpu", "value": "1002:6821"},
-                {"key": "os", "value": "Mac-10.11"},
-            ],
+            'extra_args': _SWARMING_EXTRA_ARGS,
+            'dimensions': [{'key': 'pool', 'value': 'Chrome-perf-pinpoint'}] +
+                          _SWARMING_DIMENSIONS,
             'execution_timeout_secs': '3600',
             'io_timeout_secs': '3600',
         },
-        'tags': [
-            'configuration:chromium-rel-mac11-pro',
-        ],
     }
     swarming_tasks_new.assert_called_with(body)
 
   def assertNewTaskHasBotId(self, swarming_tasks_new):
     body = {
-        'name': 'Pinpoint job on chromium-rel-mac11-pro',
+        'name': 'Pinpoint job',
         'user': 'Pinpoint',
         'priority': '100',
         'expiration_secs': '600',
         'properties': {
             'inputs_ref': {'isolated': 'input isolate hash'},
-            'extra_args': _SWARMING_TASK_EXTRA_ARGS,
+            'extra_args': _SWARMING_EXTRA_ARGS,
             'dimensions': [
                 {'key': 'pool', 'value': 'Chrome-perf-pinpoint'},
                 {'key': 'id', 'value': 'bot id'},
@@ -61,9 +60,6 @@ class _RunTestTest(unittest.TestCase):
             'execution_timeout_secs': '3600',
             'io_timeout_secs': '3600',
         },
-        'tags': [
-            'configuration:chromium-rel-mac11-pro',
-        ],
     }
     swarming_tasks_new.assert_called_with(body)
 
@@ -76,7 +72,7 @@ class RunTestFullTest(_RunTestTest):
     # Goes through a full run of two Executions.
 
     # Call RunTest.Start() to create an Execution.
-    quest = run_test.RunTest('chromium-rel-mac11-pro', 'test_suite', 'test', 1)
+    quest = run_test.RunTest(_SWARMING_DIMENSIONS, _SWARMING_EXTRA_ARGS)
     execution = quest.Start('input isolate hash')
 
     swarming_task_result.assert_not_called()
@@ -122,33 +118,6 @@ class RunTestFullTest(_RunTestTest):
     self.assertNewTaskHasBotId(swarming_tasks_new)
 
 
-@mock.patch('dashboard.services.swarming_service.Tasks.New')
-class SwarmingTaskStartTest(_RunTestTest):
-
-  def testPagesetRepeat(self, swarming_tasks_new):
-    quest = run_test.RunTest('chromium-rel-mac11-pro', 'test_suite', 'test',
10)
-    execution = quest.Start('input isolate hash')
-    execution.Poll()
-
-    new_call_body = swarming_tasks_new.call_args[0][0]
-    self.assertIn('--pageset-repeat',
new_call_body['properties']['extra_args'])
-    self.assertIn('10', new_call_body['properties']['extra_args'])
-
-  @mock.patch('dashboard.services.swarming_service.Task.Result')
-  def testUnknownConfig(self, swarming_task_result, swarming_tasks_new):
-    quest = run_test.RunTest('configuration', 'test_suite', 'test', 1)
-    execution = quest.Start('input isolate hash')
-    execution.Poll()
-
-    swarming_task_result.assert_not_called()
-    swarming_tasks_new.assert_not_called()
-    self.assertTrue(execution.completed)
-    self.assertTrue(execution.failed)
-    self.assertEqual(len(execution.result_values), 1)
-    self.assertIsInstance(execution.result_values[0],
-                          run_test.UnknownConfigError)
-
-
 @mock.patch('dashboard.services.swarming_service.Tasks.New')
 @mock.patch('dashboard.services.swarming_service.Task.Result')
 class SwarmingTaskStatusTest(_RunTestTest):
@@ -157,7 +126,7 @@ class SwarmingTaskStatusTest(_RunTestTest):
     swarming_task_result.return_value = {'state': 'BOT_DIED'}
     swarming_tasks_new.return_value = {'task_id': 'task id'}
 
-    quest = run_test.RunTest('chromium-rel-mac11-pro', 'test_suite', 'test', 1)
+    quest = run_test.RunTest(_SWARMING_DIMENSIONS, _SWARMING_EXTRA_ARGS)
     execution = quest.Start('input isolate hash')
     execution.Poll()
     execution.Poll()
@@ -177,7 +146,7 @@ class SwarmingTaskStatusTest(_RunTestTest):
     }
     swarming_tasks_new.return_value = {'task_id': 'task id'}
 
-    quest = run_test.RunTest('chromium-rel-mac11-pro', 'test_suite', 'test', 1)
+    quest = run_test.RunTest(_SWARMING_DIMENSIONS, _SWARMING_EXTRA_ARGS)
     execution = quest.Start('isolate_hash')
     execution.Poll()
     execution.Poll()
@@ -202,7 +171,7 @@ class BotIdHandlingTest(_RunTestTest):
     swarming_tasks_new.return_value = {'task_id': 'task id'}
     swarming_task_result.return_value = {'state': 'EXPIRED'}
 
-    quest = run_test.RunTest('chromium-rel-mac11-pro', 'test_suite', 'test', 1)
+    quest = run_test.RunTest(_SWARMING_DIMENSIONS, _SWARMING_EXTRA_ARGS)
     execution = quest.Start('input isolate hash')
     execution.Poll()
     execution.Poll()
@@ -226,7 +195,7 @@ class BotIdHandlingTest(_RunTestTest):
                                  swarming_tasks_new):
     # Executions after the first must wait for the first execution to get a bot
     # ID. To preserve device affinity, they must use the same bot.
-    quest = run_test.RunTest('chromium-rel-mac11-pro', 'test_suite', 'test', 1)
+    quest = run_test.RunTest(_SWARMING_DIMENSIONS, _SWARMING_EXTRA_ARGS)
     execution_1 = quest.Start('input isolate hash')
     execution_2 = quest.Start('input isolate hash')

[dtu, 2017-08-16 22:06:25]

The CQ bit was checked by dtu@chromium.org

[dtu, 2017-08-16 22:06:26]

The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/2996473002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-08-16 22:06:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-16 22:33:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1502921185942300,
"parent_rev": "0612d5614fa5b2699b38f76c7f04b7721945e9bf", "commit_rev":
"cd8447f51123eb08223731470b6f10f150b81142"}

[commit-bot: I haz the power, 2017-08-16 22:33:47]

Description was changed from

==========
[pinpoint] Add QuestGenerator object.

Telemetry and GTest tests have a few significant differences between them. The
new QuestGenerators abstract out the differences between different types of
tests. They have the ability to read different request parameters for different
test types and customize the list of Quests for each type.

BUG=catapult:#3729
==========

to

==========
[pinpoint] Add QuestGenerator object.

Telemetry and GTest tests have a few significant differences between them. The
new QuestGenerators abstract out the differences between different types of
tests. They have the ability to read different request parameters for different
test types and customize the list of Quests for each type.

BUG=catapult:#3729

Review-Url: https://codereview.chromium.org/2996473002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-08-16 22:33:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...