On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@xxxxxxxxx> wrote:
> t/t5520: modify tests to reduce common code
As this is indeed a patch, "modify" is implied. Perhaps:
t5520: factor out common code
> There exist three groups of tests which have repetitive lines of code.
>
> Introduce two functions test_rebase_autostash() and
> test_rebase_no_autostash() to reduce the number of lines. Also introduce
> loops to futher reduce the current implementation.
This patch is doing so much that it's difficult to review for
correctness. Taking [1] into consideration, better would be to split
it into at least three patches:
1. Factor out code into test_rebase_autostash() and modify the four
tests to call it.
2. Factor out code into test_rebase_autostash_fail() and modify the
three tests to call it.
3. Fold the two "pull $i (without --rebase) is illegal" tests into a for-loop.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/289434/focus=289860
> Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> Signed-off-by: Mehul Jain <mehul.jain2029@xxxxxxxxx>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -9,6 +9,24 @@ modify () {
> +test_rebase_no_autostash () {
> + git reset --hard before-rebase &&
> + echo dirty >new_file &&
> + git add new_file &&
> + test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> + test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
In the spirit of patch 3/5 and [1], you could grep for a substring
rather than the full message, but that's a minor point, not worth a
re-roll.
test_i18ngrep "uncommitted changes" err
> +}
> @@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
> +for i in true false
> + do
> + test_expect_success "pull --rebase --autostash & rebase.autostash=$i" '
> + test_config rebase.autostash $i &&
> + test_rebase_autostash
> + '
> + done
I don't care too strongly, but I'm not convinced that this for-loop is
buying you much for these two cases since each test already has been
reduced to two simple lines, and the added abstraction of the for-loop
increases cognitive load a bit.
> +for i in --autostash --no-autostash
> + do
> + test_expect_success "pull $i (without --rebase) is illegal" '
> + test_must_fail git pull $i . copy 2>actual &&
> + test_i18ngrep "only valid with --rebase" actual
> + '
> + done
You might then ask why I suggested[1] the for-loop in this case but
not for the true/false case. Even though these are also two-line
tests, they are not quite as simple as two lines down to which the
true/false tests devolve. Anyhow, this alone is not worth a re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html