Run "git stash" and "git stash apply" before and after a dry test run.

Run "git stash" and "git stash apply" before and after a dry test run.

Reason: We don't want to destroy local uncommited changes when running tests.

One solution was to mock bisect_utils.GitRun -- doing that caused the course of the run to change, making it so that there were no results (since git log is used to get a list of revisions, even for the dry run). So if we want to solve this problem by mocking git, we should replace it with a fake RunGit that returns different canned values depending on what the input is.

The advantage of stash/apply is that its simpler, but it's also a little bit slower than the above way.

BUG=

Committed: https://crrev.com/e9c4d717e1147c6e7248bc071450e62ea24530f1
Cr-Commit-Position: refs/heads/master@{#313650}

[qyearsley, 2015-01-14 22:27:32]

qyearsley@chromium.org changed reviewers:
+ prasadv@chromium.org, robertocn@chromium.org

[RobertoCN, 2015-01-14 22:36:56]

On 2015/01/14 22:28:15, qyearsley wrote:

I think we should instead make run_tests refuse to run if there are uncommitted
changes, at least until we are able to successfully mock RunGit in a way that
doesn't trivialize our tests.

[Peter Kasting, 2015-01-14 22:44:51]

Shouldn't you use git stash pop instead of git stash apply?  Otherwise you
modify the state of the stash queue.

[qyearsley, 2015-01-15 00:50:52]

On 2015/01/14 22:44:51, Peter Kasting wrote:
> Shouldn't you use git stash pop instead of git stash apply?  Otherwise you
> modify the state of the stash queue.

Good point. I never used `git stash pop` before, which according to
http://stackoverflow.com/questions/15286075/difference-between-git-stash-pop-...
is about the same as `git stash apply && git stash drop`.

There's one more problem with git stash + git stash apply/pop that Roberto
pointed out before. If there are stashed changes but not staged changes, then
the git stash + git stash pop would apply changes which weren't applied before
running the tests, which we also don't want.

Current patch set makes it just abort the test if there are staged changes.
(Note: "git cl upload" also refuses to run when there are uncommitted changes.)

[qyearsley, 2015-01-27 00:18:46]

Ping -- does this version (abort if there are uncommitted changes) seem OK?

[prasadv, 2015-01-27 22:32:32]

https://codereview.chromium.org/847393004/diff/40001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/847393004/diff/40001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:176: if changes.strip():
What does "CheckRunGit" returns when there is no changes? if None then
changes.strip() will fail.

[qyearsley, 2015-01-27 23:49:02]

https://codereview.chromium.org/847393004/diff/40001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/847393004/diff/40001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:176: if changes.strip():
On 2015/01/27 22:32:32, prasadv wrote:
> What does "CheckRunGit" returns when there is no changes? if None then
> changes.strip() will fail.

CheckRunGit returns an empty string when there are no changes, e.g.:

>>> import bisect_utils
>>> bisect_utils.RunGit(['status', '--short'])
('', 0)

So, this means calling strip() should be unnecessary. Changed it to not call
strip() and added a comment.

[prasadv, 2015-01-27 23:50:42]

lgtm

[qyearsley, 2015-01-27 23:59:23]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2015-01-28 00:01:00]

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

[commit-bot: I haz the power, 2015-01-28 00:09:22]

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

[commit-bot: I haz the power, 2015-01-28 00:09:23]

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

[qyearsley, 2015-01-28 00:46:26]

On 2015/01/28 00:09:23, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

It looks like the chromium_presubmit try job is failing, because the test is
aborting when running on the try bot:
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...

One fix for this would be to change sys.exit(1) -> sys.exit(0), but this also
means that some tests likely won't be run on chromium_presubmit try bot in the
future. Any other ideas?

[chromium-reviews, 2015-01-28 22:59:32]

I think that we should maybe print the uncommitted changes (or at least the
output of the git command) before exiting with an error code. For one,
It'll tell us why it is failing on the trybot. Depending on what we find,
we could choose to parse such output and continue regardless on certain
cases.

On Tue Jan 27 2015 at 4:46:27 PM <qyearsley@chromium.org> wrote:

> On 2015/01/28 00:09:23, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >    chromium_presubmit on tryserver.chromium.linux
>
> (http://build.chromium.org/p/tryserver.chromium.linux/
> builders/chromium_presubmit/builds/38756)
>
> It looks like the chromium_presubmit try job is failing, because the test
> is
> aborting when running on the try bot:
> http://build.chromium.org/p/tryserver.chromium.linux/
> builders/chromium_presubmit/builds/38765/steps/presubmit/logs/stdio
>
> One fix for this would be to change sys.exit(1) -> sys.exit(0), but this
> also
> means that some tests likely won't be run on chromium_presubmit try bot in
> the
> future. Any other ideas?
>
> https://codereview.chromium.org/847393004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qyearsley, 2015-01-29 01:47:34]

On 2015/01/28 22:59:32, chromium-reviews wrote:
> I think that we should maybe print the uncommitted changes (or at least the
> output of the git command) before exiting with an error code. For one,
> It'll tell us why it is failing on the trybot. Depending on what we find,
> we could choose to parse such output and continue regardless on certain
> cases.
> 

Good idea. After I did this, I got the following output when the presubmit was
running on the bot:

There are un-committed changes in the current branch.
Aborting the tests to avoid destroying local changes. Changes:
?? tools/auto_bisect/bisect.cfgc
?? tools/run-perf-test.cfgc

The "??" means that it's an untracked file change, and I'm not sure why the
filenames appear to end in ".cfgc". After changing the "git status --short" to
"git status --short --untracked-files=no", the presubmit passes.

Any concerns before submitting this?

[chromium-reviews, 2015-01-29 01:52:53]

Sounds good. cfgc is the extension from precompiled python files that
originally end in .cfg (pylint compiles python files at some point even if
they have .cfg extension)

On Wed Jan 28 2015 at 5:47:35 PM <qyearsley@chromium.org> wrote:

> On 2015/01/28 22:59:32, chromium-reviews wrote:
> > I think that we should maybe print the uncommitted changes (or at least
> > the
> > output of the git command) before exiting with an error code. For one,
> > It'll tell us why it is failing on the trybot. Depending on what we find,
> > we could choose to parse such output and continue regardless on certain
> > cases.
>
>
> Good idea. After I did this, I got the following output when the presubmit
> was
> running on the bot:
>
> There are un-committed changes in the current branch.
> Aborting the tests to avoid destroying local changes. Changes:
> ?? tools/auto_bisect/bisect.cfgc
> ?? tools/run-perf-test.cfgc
>
> The "??" means that it's an untracked file change, and I'm not sure why the
> filenames appear to end in ".cfgc". After changing the "git status --short"
> to
> "git status --short --untracked-files=no", the presubmit passes.
>
> Any concerns before submitting this?
>
> https://codereview.chromium.org/847393004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qyearsley, 2015-01-29 01:56:54]

On 2015/01/29 01:52:53, chromium-reviews wrote:
> Sounds good. cfgc is the extension from precompiled python files that
> originally end in .cfg (pylint compiles python files at some point even if
> they have .cfg extension)
> 
> On Wed Jan 28 2015 at 5:47:35 PM <mailto:qyearsley@chromium.org> wrote:
> 
> > On 2015/01/28 22:59:32, chromium-reviews wrote:
> > > I think that we should maybe print the uncommitted changes (or at least
> > > the
> > > output of the git command) before exiting with an error code. For one,
> > > It'll tell us why it is failing on the trybot. Depending on what we find,
> > > we could choose to parse such output and continue regardless on certain
> > > cases.
> >
> >
> > Good idea. After I did this, I got the following output when the presubmit
> > was
> > running on the bot:
> >
> > There are un-committed changes in the current branch.
> > Aborting the tests to avoid destroying local changes. Changes:
> > ?? tools/auto_bisect/bisect.cfgc
> > ?? tools/run-perf-test.cfgc
> >
> > The "??" means that it's an untracked file change, and I'm not sure why the
> > filenames appear to end in ".cfgc". After changing the "git status --short"
> > to
> > "git status --short --untracked-files=no", the presubmit passes.
> >
> > Any concerns before submitting this?
> >
> > https://codereview.chromium.org/847393004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ah, right -- I've seen .pyc but for some reason didn't think that the .cfg files
would also have compiled python files made for them with a "c" appended.

[qyearsley, 2015-01-29 01:56:58]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2015-01-29 01:58:09]

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

[commit-bot: I haz the power, 2015-01-29 02:37:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-29 02:38:25]

Patchset 6 (id:??) landed as
https://crrev.com/e9c4d717e1147c6e7248bc071450e62ea24530f1
Cr-Commit-Position: refs/heads/master@{#313650}