Patchwork [2,of,3,stable] tests: correct (I think) command in test-largefiles-update

login
register
mail settings
Submitter Augie Fackler
Date Jan. 30, 2017, 11:54 p.m.
Message ID <b4118549138f0872c218.1485820457@imladris.local>
Download mbox | patch
Permalink /patch/18285/
State Accepted
Headers show

Comments

Augie Fackler - Jan. 30, 2017, 11:54 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1485817397 18000
#      Mon Jan 30 18:03:17 2017 -0500
# Branch stable
# Node ID b4118549138f0872c218835d3ae10c9e70d867de
# Parent  d717001c36eb9d184b6c83874feb600adc22e8e7
tests: correct (I think) command in test-largefiles-update

When this test was introduced, it used the short-form of all the flags
on this update invocation. I suspect, based on the "start with clean
dirstates" comment and the fact that the no-exec branch of the #if
guard leaves dirstate clean, that this should have been 'update -qCr'
instead of 'update -qcr', but that a bug in largefiles --check
handling left this problem unnoticed.

I'll leave a breadcrumb further up about the current failure mode in
the hopes that we can fix this some day.

This was previously discussed in [0] but the trail in that thread goes
cold after a few replies. Given that this is still a flaky test, that
appears to only be passing by bad fortune, I think it's worth
correcting the code of the test to make a correct assertion, and to
keep track of the suspected bug with some other mechanism than an
invalid test (if we had support for "expected failure" blocks this
might be a worthwhile use of them?).

0: https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089501.html
Yuya Nishihara - Jan. 31, 2017, 3:07 p.m.
On Mon, 30 Jan 2017 18:54:17 -0500, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1485817397 18000
> #      Mon Jan 30 18:03:17 2017 -0500
> # Branch stable
> # Node ID b4118549138f0872c218835d3ae10c9e70d867de
> # Parent  d717001c36eb9d184b6c83874feb600adc22e8e7
> tests: correct (I think) command in test-largefiles-update

These look good. Queued the first two, thanks.
Mads Kiilerich - Feb. 1, 2017, 12:50 a.m.
On 01/31/2017 12:54 AM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1485817397 18000
> #      Mon Jan 30 18:03:17 2017 -0500
> # Branch stable
> # Node ID b4118549138f0872c218835d3ae10c9e70d867de
> # Parent  d717001c36eb9d184b6c83874feb600adc22e8e7
> tests: correct (I think) command in test-largefiles-update
>
> When this test was introduced, it used the short-form of all the flags
> on this update invocation. I suspect, based on the "start with clean
> dirstates" comment and the fact that the no-exec branch of the #if
> guard leaves dirstate clean, that this should have been 'update -qCr'
> instead of 'update -qcr', but that a bug in largefiles --check
> handling left this problem unnoticed.

My initial intent was to use --check. I (incorrectly) assumed it would 
be clean in both cases and just wanted to make that assumption explicit. 
It thus seem justified to use --clean to cover the no-execbit case.

> I'll leave a breadcrumb further up about the current failure mode in
> the hopes that we can fix this some day.
>
> This was previously discussed in [0] but the trail in that thread goes
> cold after a few replies. Given that this is still a flaky test, that
> appears to only be passing by bad fortune, I think it's worth
> correcting the code of the test to make a correct assertion, and to
> keep track of the suspected bug with some other mechanism than an
> invalid test (if we had support for "expected failure" blocks this
> might be a worthwhile use of them?).
>
> 0: https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089501.html
>
> diff --git a/tests/test-largefiles-update.t b/tests/test-largefiles-update.t
> --- a/tests/test-largefiles-update.t
> +++ b/tests/test-largefiles-update.t
> @@ -737,7 +737,7 @@ files correctly after an interrupted upd
>   hashes reveals it isn't clean.
>   
>   Start with clean dirstates:
> -  $ hg up --quiet --check --rev "8^"
> +  $ hg up --quiet --clean --rev "8^"

IIRC, some of the random failures I have seen wasn't in --check failing 
the check. Instead, later operations showed that this --check silently 
had failed to detect changes. Turning it into --clean would thus not 
make any difference - it would not find any changes to clean away.

That is why my patch also removed the largefile and standin so I was 
sure --clean would restore the state of the file.

Things might have changed since then, and this --clean might now be 
enough to make it stable. If not, this might be why.

I like that your next patch adds a FIXME ... but of course not that it 
also re-introduce the instability.

/Mads
Augie Fackler - Feb. 1, 2017, 7:09 p.m.
> On Jan 31, 2017, at 19:50, Mads Kiilerich <mads@kiilerich.com> wrote:
> 
> IIRC, some of the random failures I have seen wasn't in --check failing the check. Instead, later operations showed that this --check silently had failed to detect changes. Turning it into --clean would thus not make any difference - it would not find any changes to clean away.
> 
> That is why my patch also removed the largefile and standin so I was sure --clean would restore the state of the file.
> 
> Things might have changed since then, and this --clean might now be enough to make it stable. If not, this might be why.
> 
> I like that your next patch adds a FIXME ... but of course not that it also re-introduce the instability.

Yeah. Maybe I should file a bug for now?

Patch

diff --git a/tests/test-largefiles-update.t b/tests/test-largefiles-update.t
--- a/tests/test-largefiles-update.t
+++ b/tests/test-largefiles-update.t
@@ -737,7 +737,7 @@  files correctly after an interrupted upd
 hashes reveals it isn't clean.
 
 Start with clean dirstates:
-  $ hg up --quiet --check --rev "8^"
+  $ hg up --quiet --clean --rev "8^"
   $ sleep 1
   $ hg st
 Update standins without updating largefiles - large1 is modified and largeX is