git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 2/3] rebase: Add tests for console output
Date: Wed, 7 Jun 2017 11:47:02 +0100	[thread overview]
Message-ID: <5309bbe7-edd8-0c34-82ff-c3162d7d9e66@talktalk.net> (raw)
In-Reply-To: <alpine.DEB.2.21.1.1706011329350.3610@virtualbox>

Hi Johannes

Thanks for your feedback
On 01/06/17 13:56, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 31 May 2017, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Check the console output when using --autostash and the stash applies
>> cleanly is what we expect. To avoid this test depending on commit and
>> stash hashes it uses sed to replace them with XXX. The sed script also
>> replaces carriage returns in the output with '\r' to avoid embedded
>> '^M's in the expected output files. Unfortunately this means we still
>> end up with an embedded '^M' in the sed script which may not be
>> preserved when sending this. The last line of the sed script should be
>> +s/^M/\\r/g
> 
> Like Junio pointed out, this sed script would not be portable. To
> elaborate: there are two major variants of sed out there, GNU sed and BSD
> sed. Typically GNU sed allows a little more powerful instructions, e.g. \t
> and \r.

I'm confused by this as my script does not use the escape sequence "\r"
out of portability concerns. It has a literal carriage return as you get
from typing Ctrl-Q Ctrl-M in emacs or Ctrl-V Ctrl-M in bash which I
think should be portable and replaces the carriage returns in git's
output with the literal string '\r'. I did this so that the expected
files don't have control characters in them that mess up the display
when you cat them or read them in an email client

> But we should simply take a step back and ask why test_cmp is not enough
> to ignore the CRs in the output?
> 
> Also, about the commit IDs. As long as the tests are consistent (i.e. they
> use test_commit rather than plain `git commit`, or at least call
> `test_tick` beforehand), the commit IDs should actually be identical
> between runs and not depend on the time of day or the date.
> 
> The only exception to that rule is when some previous test cases call
> `test_commit` but are guarded behind some prereq and may not be executed
> at all. In that case, the precise commit IDs depend on the particular set
> of active prereqs.

The other exceptions are when the commit hash algorithm changes or the
contents of the commits changes because of some update to the test
script. That's why I didn't want to rely on matching fixed SHA1s

> 
> But as far as I can tell, t3420 does not have any test cases guarded by
> prereqs.
> 
> Taking an additional step back, I wonder whether we have to hard-code the
> commit IDs (or XXX) to begin with. Why not generate the `expect` files
> with the information at hand? We can simply ask `git rev-parse --short`.
> 
> For the stash's commit ID, there is no record in the ref space, of course
> (because it was created with `git stash create`). But I think in this
> case, it is legitimate to simply grep the output.

That's a good approach to handling the stash hash if we want to generate
the expected files from the test script

> That way, the test would be precise and resilient.
> 
> So for example instead of adding the t/t3420/expected-success-am verbatim,
> you could generate the output via
> 
> 	cat >expect <<-EOF
> 	$(grep "^Created autostash: [0-9a-f][0-9a-f]*\\$" output)
> 	HEAD is now at $(git rev-parse --short HEAD~2) third commit
> 	First, rewinding head to replay your work on top of it...
> 	Applying: second commit
> 	Applying: third commit
> 	Applied autostash.
> 	EOF

This approach works well for the cases where there aren't control
characters embedded in git's output, but for the interactive tests there
are so we'd end up with control characters in the test script which I
wanted to avoid or doing $(printf '\r'). I steered clear of generating
the expected file in the test as i) it was more work (both for me
(rebase --merge has a few commit hashes in it's output) and when the
script is running) and ii) it's a bit messy to implement given the way
the tests are structured in a helper function that's called with a
parameter indicating the type of rebase to test.

I can go ahead with generating the expected files from the script if you
really want but I wonder if changing the test to generate the sed script
with printf as below might be a simpler way to mitigate the carriage
return problem, though it would be less strict than generating the real
hashes with rev-parse.

--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -189,6 +189,16 @@ test_expect_success "rebase: noop rebase" '
 	git checkout feature-branch
 '

+test_expect_sucess 'rebase: create sed script to sanitize output' '
+	printf "\
+s/^\(Created autostash: \)[0-9a-f]\{6,\}$/\1XXX/
+s/^\(HEAD is now at \)[0-9a-f]\{6,\}\( .* commit\)$/\1XXX\2/
+s/^[0-9a-f]\{6,\}\( .* commit\)$/XXX\1/
+s/\(detached HEAD \)[0-9a-f]\{6,\}/\1XXX/
+s/\(could not apply \)[0-9a-f]\{6,\}/\1XXX/g
+s/\\r/\\r/g" >$TEST_DIRECTORY/t4320/remove-ids.sed
+'
+

Let me know what you think,

Best Wishes

Phillip


  parent reply	other threads:[~2017-06-07 10:47 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-31 10:42 [PATCH 0/3] Add regression tests for recent rebase -i fixes Phillip Wood
2017-05-31 10:42 ` [PATCH 1/3] rebase -i: Add test for reflog message Phillip Wood
2017-06-01  2:00   ` Junio C Hamano
2017-05-31 10:42 ` [PATCH 2/3] rebase: Add tests for console output Phillip Wood
2017-05-31 19:02   ` Phillip Wood
2017-06-01  1:59     ` Junio C Hamano
2017-06-01 12:56   ` Johannes Schindelin
2017-06-01 23:40     ` Junio C Hamano
2017-06-01 23:47       ` Stefan Beller
2017-06-02 12:47         ` pushing for a new hash, was " Johannes Schindelin
2017-06-02 17:54           ` Jonathan Nieder
2017-06-02 18:05             ` Jonathan Nieder
2017-06-02 20:29             ` Ævar Arnfjörð Bjarmason
2017-06-15 10:38               ` Johannes Schindelin
2017-06-03  0:36             ` Junio C Hamano
2017-06-06 22:22             ` Johannes Schindelin
2017-06-06 22:45               ` Jonathan Nieder
2017-06-07  1:09                 ` Junio C Hamano
2017-06-07  2:18                   ` [PATCH] t4005: modernize style and drop hard coded sha1 Stefan Beller
2017-06-07 17:39                     ` Brandon Williams
2017-06-06 22:45               ` pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output Stefan Beller
2017-06-06 22:52                 ` Jonathan Nieder
2017-06-07  0:34                 ` Samuel Lijin
2017-06-07 14:47                 ` Johannes Schindelin
2017-06-07 16:53                   ` Stefan Beller
2017-06-07 10:47     ` Phillip Wood [this message]
2017-06-09 16:39       ` Junio C Hamano
2017-06-14 10:18         ` Phillip Wood
2017-06-14 12:51       ` Johannes Schindelin
2017-05-31 10:42 ` [PATCH 3/3] rebase: Add tests for console output with conflicting stash Phillip Wood
2017-06-14 10:24 ` [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes Phillip Wood
2017-06-14 10:24   ` [PATCH v2 1/3] rebase -i: Add test for reflog message Phillip Wood
2017-06-14 10:24   ` [PATCH v2 2/3] rebase: Add regression tests for console output Phillip Wood
2017-06-14 10:24   ` [PATCH v2 3/3] rebase: Add more " Phillip Wood
2017-06-14 20:35   ` [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes Johannes Schindelin
2017-06-15 23:05   ` Junio C Hamano
2017-06-15 23:23     ` Junio C Hamano
2017-06-15 23:29       ` Junio C Hamano
2017-06-16 13:49         ` Johannes Schindelin
2017-06-16 18:43           ` Johannes Sixt
2017-06-16 21:05             ` Junio C Hamano
2017-06-19 19:45             ` Johannes Sixt
2017-06-19 20:02               ` Junio C Hamano
2017-06-19  9:49           ` Phillip Wood
2017-06-19 15:45             ` Junio C Hamano
2017-06-19  9:52         ` Phillip Wood
2017-06-19 17:56 ` [PATCH v3 0/4] Add regression tests for recent " Phillip Wood
2017-06-19 17:56   ` [PATCH v3 1/4] sequencer: print autostash messages to stderr Phillip Wood
2017-06-19 17:56   ` [PATCH v3 2/4] rebase -i: Add test for reflog message Phillip Wood
2017-06-19 17:56   ` [PATCH v3 3/4] rebase: Add regression tests for console output Phillip Wood
2017-06-19 17:56   ` [PATCH v3 4/4] rebase: Add more " Phillip Wood
2017-06-23  4:17   ` [PATCH v3 0/4] Add regression tests for recent rebase -i fixes Junio C Hamano
2017-06-23  5:07     ` Junio C Hamano
2017-06-23  9:53       ` Phillip Wood
2017-06-23 17:03         ` Junio C Hamano
2017-06-23 18:53           ` Junio C Hamano
2017-06-26  9:17             ` Phillip Wood
2017-06-23 19:01           ` Junio C Hamano
2017-06-26  9:23             ` Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5309bbe7-edd8-0c34-82ff-c3162d7d9e66@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).