Use git cl try --properties option to send test arguments instead of run-perf-test.cfg

Use git cl try --properties option to send test arguments instead of run-perf-test.cfg

BUG=636507
Committed: https://crrev.com/62facebbb3a84eb18d7964207b70be380320ddbd
Cr-Commit-Position: refs/heads/master@{#415801}

[prasadv, 2016-08-29 23:06:02]

prasadv@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com, sullivan@chromium.org

[nednguyen, 2016-08-29 23:15:22]

lg2me overall with some comments

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
File tools/perf/core/trybot_command.py (right):

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:309: print error
Why print error then return 1? Wouldn't not catching the exception will give the
same effect with the clearer stack trace?

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:435: 'CL for %s perf tryjob' % repo_name],
nits: would be nice to include benchmark name & bots in the message. 

Also would the title be the same as the message or do we need '-t' option as
well?

[eakuefner, 2016-08-30 17:33:50]

lgtm with a few nits

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
File tools/perf/core/trybot_command.py (right):

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:138: """Runs the GIT command with the given
arguments.
nit: s/GIT/git/

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:143: ignore_return_code: Ignores the return
code for GIT command
nit: end with period?

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:152: [_GIT_CMD] + cmd, stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
Can you just write [_GIT_CMD, cmd]?

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:369: TrybotError: This exception is raised for
the following cases,
nit: cases:

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:435: 'CL for %s perf tryjob' % repo_name],
On 2016/08/29 at 23:15:22, nednguyen wrote:
> nits: would be nice to include benchmark name & bots in the message. 
> 
> Also would the title be the same as the message or do we need '-t' option as
well?

The title should be the same as the first/only line of the message for new CLs.

[prasadv, 2016-08-30 18:49:40]

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
File tools/perf/core/trybot_command.py (right):

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:138: """Runs the GIT command with the given
arguments.
On 2016/08/30 17:33:49, eakuefner wrote:
> nit: s/GIT/git/

Done.

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:143: ignore_return_code: Ignores the return
code for GIT command
On 2016/08/30 17:33:49, eakuefner wrote:
> nit: end with period?

Done.

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:152: [_GIT_CMD] + cmd, stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
On 2016/08/30 17:33:50, eakuefner wrote:
> Can you just write [_GIT_CMD, cmd]?

cmd is a list of arguments for git command and I think subprocess.Popen expects
all tis argurmenst as list of strings.

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:309: print error
On 2016/08/29 23:15:22, nednguyen wrote:
> Why print error then return 1? Wouldn't not catching the exception will give
the
> same effect with the clearer stack trace?

Most of the TryjobError exceptions are raised when something goes wrong with git
commands in the tool. The reason to print the error is to display more meaning
error message in this exception is raised.

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:369: TrybotError: This exception is raised for
the following cases,
On 2016/08/30 17:33:49, eakuefner wrote:
> nit: cases:

Done.

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:435: 'CL for %s perf tryjob' % repo_name],
On 2016/08/30 17:33:50, eakuefner wrote:
> On 2016/08/29 at 23:15:22, nednguyen wrote:
> > nits: would be nice to include benchmark name & bots in the message. 
> > 
> > Also would the title be the same as the message or do we need '-t' option as
> well?
> 
> The title should be the same as the first/only line of the message for new
CLs.
Yes, CL title will be the first line of the message.
This raises a new question:
Before this change we used to create a new branch to upload and run the try job.
The rietveld issue created for tryjob was specific to the new branch. But with
this change the rietveld issue is created in users working branch.
So if the user choose to send his/her patch for review in their working branch,
they are required to change the title of the issue or reset the issue number and
upload the patch again.
I'm not sure whether this is acceptable to user.

[nednguyen, 2016-08-30 18:55:23]

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
File tools/perf/core/trybot_command.py (right):

https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
tools/perf/core/trybot_command.py:435: 'CL for %s perf tryjob' % repo_name],
On 2016/08/30 18:49:40, prasadv wrote:
> On 2016/08/30 17:33:50, eakuefner wrote:
> > On 2016/08/29 at 23:15:22, nednguyen wrote:
> > > nits: would be nice to include benchmark name & bots in the message. 
> > > 
> > > Also would the title be the same as the message or do we need '-t' option
as
> > well?
> > 
> > The title should be the same as the first/only line of the message for new
> CLs.
> Yes, CL title will be the first line of the message.
> This raises a new question:
> Before this change we used to create a new branch to upload and run the try
job.
> The rietveld issue created for tryjob was specific to the new branch. But with
> this change the rietveld issue is created in users working branch.
> So if the user choose to send his/her patch for review in their working
branch,
> they are required to change the title of the issue or reset the issue number
and
> upload the patch again.
> I'm not sure whether this is acceptable to user.

It seems not very ideal that this overwrites existing CL of user. Can we do a
"git cl issue 0" before do "git cl upload"?

[prasadv, 2016-08-30 19:13:18]

On 2016/08/30 18:55:23, nednguyen wrote:
>
https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
> File tools/perf/core/trybot_command.py (right):
> 
>
https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
> tools/perf/core/trybot_command.py:435: 'CL for %s perf tryjob' % repo_name],
> On 2016/08/30 18:49:40, prasadv wrote:
> > On 2016/08/30 17:33:50, eakuefner wrote:
> > > On 2016/08/29 at 23:15:22, nednguyen wrote:
> > > > nits: would be nice to include benchmark name & bots in the message. 
> > > > 
> > > > Also would the title be the same as the message or do we need '-t'
option
> as
> > > well?
> > > 
> > > The title should be the same as the first/only line of the message for new
> > CLs.
> > Yes, CL title will be the first line of the message.
> > This raises a new question:
> > Before this change we used to create a new branch to upload and run the try
> job.
> > The rietveld issue created for tryjob was specific to the new branch. But
with
> > this change the rietveld issue is created in users working branch.
> > So if the user choose to send his/her patch for review in their working
> branch,
> > they are required to change the title of the issue or reset the issue number
> and
> > upload the patch again.
> > I'm not sure whether this is acceptable to user.
> 
> It seems not very ideal that this overwrites existing CL of user. Can we do a
> "git cl issue 0" before do "git cl upload"?

I don't think this will overwrite existing CL of user, but it is other way
round.

Case 1: User did not upload the CL and runs perf try job:
  In this case the tool will upload the CL and the title is set to "CL for src
try job ..."
  So user might need to overwrite the title and description of the CL
   
Case 2: User uploaded the CL and then runs perf try job:
  In this case a patch is added to the existing CL and adds a message "CL for
src try job ..."

[nednguyen, 2016-08-30 20:31:19]

On 2016/08/30 19:13:18, prasadv wrote:
> On 2016/08/30 18:55:23, nednguyen wrote:
> >
>
https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
> > File tools/perf/core/trybot_command.py (right):
> > 
> >
>
https://codereview.chromium.org/2287993004/diff/1/tools/perf/core/trybot_comm...
> > tools/perf/core/trybot_command.py:435: 'CL for %s perf tryjob' % repo_name],
> > On 2016/08/30 18:49:40, prasadv wrote:
> > > On 2016/08/30 17:33:50, eakuefner wrote:
> > > > On 2016/08/29 at 23:15:22, nednguyen wrote:
> > > > > nits: would be nice to include benchmark name & bots in the message. 
> > > > > 
> > > > > Also would the title be the same as the message or do we need '-t'
> option
> > as
> > > > well?
> > > > 
> > > > The title should be the same as the first/only line of the message for
new
> > > CLs.
> > > Yes, CL title will be the first line of the message.
> > > This raises a new question:
> > > Before this change we used to create a new branch to upload and run the
try
> > job.
> > > The rietveld issue created for tryjob was specific to the new branch. But
> with
> > > this change the rietveld issue is created in users working branch.
> > > So if the user choose to send his/her patch for review in their working
> > branch,
> > > they are required to change the title of the issue or reset the issue
number
> > and
> > > upload the patch again.
> > > I'm not sure whether this is acceptable to user.
> > 
> > It seems not very ideal that this overwrites existing CL of user. Can we do
a
> > "git cl issue 0" before do "git cl upload"?
> 
> I don't think this will overwrite existing CL of user, but it is other way
> round.
>
 
> Case 1: User did not upload the CL and runs perf try job:
>   In this case the tool will upload the CL and the title is set to "CL for src
> try job ..."
>   So user might need to overwrite the title and description of the CL

I think this is ok. When user does "git cl upload" to send out the patch for
reviewing, they need to write their own title & description anyway. We don't add
extra work for them with this.

>    
> Case 2: User uploaded the CL and then runs perf try job:
>   In this case a patch is added to the existing CL and adds a message "CL for
> src try job ..."
What I meant was this case. I thought -m will override the title & description
of existing CL. This seem ok to me.

[nednguyen, 2016-08-30 20:33:24]

lgtm

[prasadv, 2016-08-30 22:13:08]

The CQ bit was checked by prasadv@chromium.org

[prasadv, 2016-08-30 22:13:08]

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

[commit-bot: I haz the power, 2016-08-30 22:14:01]

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, 2016-08-30 23:35:29]

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

[commit-bot: I haz the power, 2016-08-30 23:35:30]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[prasadv, 2016-08-30 23:58:46]

On 2016/08/30 23:35:30, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Duh! Blunder! in test cases, used my local path.
I'll upload a fix to this patch

[prasadv, 2016-08-31 17:51:06]

The CQ bit was checked by prasadv@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 17:51:58]

Dry run: 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, 2016-08-31 19:05:49]

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

[commit-bot: I haz the power, 2016-08-31 19:05:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[prasadv, 2016-08-31 20:37:20]

The CQ bit was checked by prasadv@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 20:37:57]

Dry run: 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, 2016-08-31 22:57:05]

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

[commit-bot: I haz the power, 2016-08-31 22:57:06]

Dry run: This issue passed the CQ dry run.

[prasadv, 2016-08-31 23:19:57]

The CQ bit was checked by prasadv@chromium.org

[prasadv, 2016-08-31 23:19:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2287993004/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-08-31 23:20:27]

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, 2016-08-31 23:26:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-31 23:27:45]

Description was changed from

==========
Use git cl try --properties option to send test arguments instead of
run-perf-test.cfg

BUG=636507
==========

to

==========
Use git cl try --properties option to send test arguments instead of
run-perf-test.cfg

BUG=636507

Committed: https://crrev.com/62facebbb3a84eb18d7964207b70be380320ddbd
Cr-Commit-Position: refs/heads/master@{#415801}
==========

[commit-bot: I haz the power, 2016-08-31 23:27:47]

Patchset 5 (id:??) landed as
https://crrev.com/62facebbb3a84eb18d7964207b70be380320ddbd
Cr-Commit-Position: refs/heads/master@{#415801}