Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-11 Thread Duy Nguyen 
On Thu, Jan 11, 2018 at 3:24 AM, Johannes Schindelin
 wrote:
>> diff --git a/Makefile b/Makefile
>> index 2a81ae22e9..567387b558 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -644,6 +644,7 @@ X =
>>
>>  PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>>
>> +TEST_PROGRAMS_NEED_X += test-3071-wildmatch
>
> I guess I can always work on unifying those gazillion of test executables
> into a single one later.

Oh yeah. I did notice your remark about disk consumption but this was
a quick hack that I would not bother with it. For the record I'm
slightly bothered with many test programs too, not due to disk size
but because it increases link time (disk i/o probably also plays part
in that). This may help another thing... at the end of the mail

>> +static struct match_input match_tests[] = {
>> + /* Basic wildmatch features */
>> + { 1, "foo", "foo" },
>> + { 0, "foo", "bar" },
>> + { 1, "", "" },
>
> These patterns share the "magic-ness" of Ævar's test cases... although
> your version is certainly more concise.

Another thing will make me move away from this style is, you can't
mark one test broken. In the end, we may have some macro that issue
one match() call per line, very similar to how t3070 does now. Then we
have more freedom in marking tests.

> BTW IIRC Ævar explicitly said that he needs to use `ls-files` in order to
> test the interaction with the index, so that would probably take a little
> bit more work.

Yeah, run_command() and stuff, not super hard (but then it opens up
another aspect I didn't address in this quick hack: collecting output
log of a test and only showing it when the test fails, could be
tricker to do in C than shell.

>> diff --git a/t/t3071-wildmatch.sh b/t/t3071-wildmatch.sh
>> new file mode 100755
>> index 00..6e83b4d684
>> --- /dev/null
>> +++ b/t/t3071-wildmatch.sh
>> @@ -0,0 +1,3 @@
>> +#!/bin/sh
>> +
>> +exec helper/test-3071-wildmatch t3071-wildmatch "$@"
>
> Should it not be `exec test-3071-wildmatch "${0%.sh}" "$@"`?

No, test-lib.sh is required to set up $PATH properly so you can run
test programs without path. This is another sticky point. Some
integration with test-lib.sh is needed. I would like to have something
like this

-- 8< --
cat >t3071-wildmatch-c.sh

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-10 Thread Johannes Schindelin 
Hi Duy,

On Wed, 10 Jan 2018, Duy Nguyen wrote:

> On Mon, Jan 08, 2018 at 01:25:04PM +0100, Johannes Schindelin wrote:
> > I agree that it would make a ton of sense to use a proper, portable test
> > framework written in pure, portable C.
> > 
> > However, this ship has long sailed, hasn't it?
> 
> If you meant converting the whole test suite, oh yeah that's not gonna
> happen. But it's still possible to have some tests written in C.

True.

> I played a bit with this. The assumption is if it's agreed that we can
> get something bare bone (but functional) in then we could start having
> more and more C-based unit tests in future and also improve the C
> framework to be on par with shell one on the side.

We can also start with something small that does not contend to replace
the entire test suite, and evolve from there as we have time.

Your initial patch looks very good, I will give it a cursory review (only
cursory because we are technically in a feature-freeze...)

> diff --git a/Makefile b/Makefile
> index 2a81ae22e9..567387b558 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -644,6 +644,7 @@ X =
>  
>  PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>  
> +TEST_PROGRAMS_NEED_X += test-3071-wildmatch

I guess I can always work on unifying those gazillion of test executables
into a single one later.

For testing purposes, I have to bundle them (BusyBox-based MinGit is
supposed to be stand-alone, yet I want to test it to verify that it works
even if it ships only a subset of Git for Windows), and they dominate the
payload of any prerelease, as you can see e.g. here:
https://github.com/git-for-windows/git/releases/tag/v2.16.0-rc1.windows.1

> diff --git a/t/helper/test-3071-wildmatch.c b/t/helper/test-3071-wildmatch.c
> new file mode 100644
> index 00..24a657202d
> --- /dev/null
> +++ b/t/helper/test-3071-wildmatch.c
> @@ -0,0 +1,273 @@
> +#include "cache.h"
> +#include "test-lib.h"
> +
> +struct match_input {
> + int expect_true;
> + const char *text;
> + const char *pattern;
> +};
> +
> +static struct match_input match_tests[] = {
> + /* Basic wildmatch features */
> + { 1, "foo", "foo" },
> + { 0, "foo", "bar" },
> + { 1, "", "" },

These patterns share the "magic-ness" of Ævar's test cases... although
your version is certainly more concise.

BTW IIRC Ævar explicitly said that he needs to use `ls-files` in order to
test the interaction with the index, so that would probably take a little
bit more work.

> diff --git a/t/t3071-wildmatch.sh b/t/t3071-wildmatch.sh
> new file mode 100755
> index 00..6e83b4d684
> --- /dev/null
> +++ b/t/t3071-wildmatch.sh
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +
> +exec helper/test-3071-wildmatch t3071-wildmatch "$@"

Should it not be `exec test-3071-wildmatch "${0%.sh}" "$@"`?

> diff --git a/test-lib.c b/test-lib.c
> new file mode 100644
> index 00..8e8b7cd6df
> --- /dev/null
> +++ b/test-lib.c
> @@ -0,0 +1,97 @@
> [...]

Lots of good stuff in there. Definitely a good start.

Ciao,
Dscho

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-10 Thread Duy Nguyen 
On Wed, Jan 10, 2018 at 5:38 PM, Adam Dinwoodie  wrote:
>> One disadvantage of this though, if this kind of framework does not
>> get popular, then any new test feature must be added at both places
>> but it's a waste of time to support both. So...
>
> I don't follow: if we end up implementing every test twice, as we have
> here, then I agree, but I don't think there's much value in doing that
> except as a proof of concept, as in this immediate discussion.  The
> obvious-to-me way to do this would be following the precedent of the
> core code: gradually migrate things away from shell code to C code.

Not the tests themselves. Test features, like --valgrind, --debug,
--verbose and that kind of stuff. These are handled by test-lib.sh. If
we add support for --new-fancy-thing to test-lib.sh then we need some
more code in test-lib.c as well.
-- 
Duy

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-10 Thread Adam Dinwoodie 
On Wednesday 10 January 2018 at 04:07 pm +0700, Duy Nguyen wrote:
> On Mon, Jan 08, 2018 at 01:25:04PM +0100, Johannes Schindelin wrote:
> > I agree that it would make a ton of sense to use a proper, portable test
> > framework written in pure, portable C.
> > 
> > However, this ship has long sailed, hasn't it?
> 
> If you meant converting the whole test suite, oh yeah that's not gonna
> happen. But it's still possible to have some tests written in C.
> 
> I played a bit with this. The assumption is if it's agreed that we can
> get something bare bone (but functional) in then we could start having
> more and more C-based unit tests in future and also improve the C
> framework to be on par with shell one on the side.
> 
> There are still some minor problems with my patch, and a bunch of
> optional features not supported. But the numbers looks unexpectedly
> promising. 0.7 seconds on the shell version and 0.03 on the C one.

I see even more promising results from a single run on one of my Cygwin
systems: from 21.3 seconds for t3070 to 0.112 seconds for your t3071.  I
expect there's a similar improvement in Dscho's Git for Windows
environment.

> One disadvantage of this though, if this kind of framework does not
> get popular, then any new test feature must be added at both places
> but it's a waste of time to support both. So...

I don't follow: if we end up implementing every test twice, as we have
here, then I agree, but I don't think there's much value in doing that
except as a proof of concept, as in this immediate discussion.  The
obvious-to-me way to do this would be following the precedent of the
core code: gradually migrate things away from shell code to C code.

Adam

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-10 Thread Duy Nguyen 
On Mon, Jan 08, 2018 at 01:25:04PM +0100, Johannes Schindelin wrote:
> I agree that it would make a ton of sense to use a proper, portable test
> framework written in pure, portable C.
> 
> However, this ship has long sailed, hasn't it?

If you meant converting the whole test suite, oh yeah that's not gonna
happen. But it's still possible to have some tests written in C.

I played a bit with this. The assumption is if it's agreed that we can
get something bare bone (but functional) in then we could start having
more and more C-based unit tests in future and also improve the C
framework to be on par with shell one on the side.

There are still some minor problems with my patch, and a bunch of
optional features not supported. But the numbers looks unexpectedly
promising. 0.7 seconds on the shell version and 0.03 on the C one.

One disadvantage of this though, if this kind of framework does not
get popular, then any new test feature must be added at both places
but it's a waste of time to support both. So...

Anyway here it is. t3071 is the same as t3070 (this is on master)

 Makefile |   2 +
 t/helper/test-3071-wildmatch.c (new) | 273 
++
 t/t3071-wildmatch.sh (new +x)|   3 +
 test-lib.c (new) |  97 ++
 test-lib.h (new) |   5 +

-- 8< --
diff --git a/Makefile b/Makefile
index 2a81ae22e9..567387b558 100644
--- a/Makefile
+++ b/Makefile
@@ -644,6 +644,7 @@ X =
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
+TEST_PROGRAMS_NEED_X += test-3071-wildmatch
 TEST_PROGRAMS_NEED_X += test-chmtime
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-config
@@ -895,6 +896,7 @@ LIB_OBJS += sub-process.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += tempfile.o
+LIB_OBJS += test-lib.o
 LIB_OBJS += tmp-objdir.o
 LIB_OBJS += trace.o
 LIB_OBJS += trailer.o
diff --git a/t/helper/test-3071-wildmatch.c b/t/helper/test-3071-wildmatch.c
new file mode 100644
index 00..24a657202d
--- /dev/null
+++ b/t/helper/test-3071-wildmatch.c
@@ -0,0 +1,273 @@
+#include "cache.h"
+#include "test-lib.h"
+
+struct match_input {
+   int expect_true;
+   const char *text;
+   const char *pattern;
+};
+
+static struct match_input match_tests[] = {
+   /* Basic wildmatch features */
+   { 1, "foo", "foo" },
+   { 0, "foo", "bar" },
+   { 1, "", "" },
+   { 1, "foo", "???" },
+   { 0, "foo", "??" },
+   { 1, "foo", "*" },
+   { 1, "foo", "f*" },
+   { 0, "foo", "*f" },
+   { 1, "foo", "*foo*" },
+   { 1, "foobar", "*ob*a*r*" },
+   { 1, "aaabababab", "*ab" },
+   { 1, "foo*", "foo\\*" },
+   { 0, "foobar", "foo\\*bar" },
+   { 1, "f\\oo", "foo" },
+   { 1, "ball", "*[al]?" },
+   { 0, "ten", "[ten]" },
+   { 0, "ten", "**[!te]" },
+   { 0, "ten", "**[!ten]" },
+   { 1, "ten", "t[a-g]n" },
+   { 0, "ten", "t[!a-g]n" },
+   { 1, "ton", "t[!a-g]n" },
+   { 1, "ton", "t[^a-g]n" },
+   { 1, "a]b", "a[]]b" },
+   { 1, "a-b", "a[]-]b" },
+   { 1, "a]b", "a[]-]b" },
+   { 0, "aab", "a[]-]b" },
+   { 1, "aab", "a[]a-]b" },
+   { 1, "]", "]" },
+
+   /* Extended slash-matching features */
+   { 0, "foo/baz/bar", "foo*bar" },
+   { 0, "foo/baz/bar", "foo**bar" },
+   { 0, "foobazbar", "foo**bar" },
+   { 1, "foo/baz/bar", "foo/**/bar" },
+   { 1, "foo/baz/bar", "foo/**/**/bar" },
+   { 1, "foo/b/a/z/bar", "foo/**/bar" },
+   { 1, "foo/b/a/z/bar", "foo/**/**/bar" },
+   { 1, "foo/bar", "foo/**/bar" },
+   { 1, "foo/bar", "foo/**/**/bar" },
+   { 0, "foo/bar", "foo?bar" },
+   { 0, "foo/bar", "foo[/]bar" },
+   { 0, "foo/bar", "foo[^a-z]bar" },
+   { 0, "foo/bar", "f[^eiu][^eiu][^eiu][^eiu][^eiu]r" },
+   { 1, "foo-bar", "f[^eiu][^eiu][^eiu][^eiu][^eiu]r" },
+   { 1, "foo", "**/foo" },
+   { 1, "XXX/foo", "**/foo" },
+   { 1, "bar/baz/foo", "**/foo" },
+   { 0, "bar/baz/foo", "*/foo" },
+   { 0, "foo/bar/baz", "**/bar*" },
+   { 1, "deep/foo/bar/baz", "**/bar/*" },
+   { 0, "deep/foo/bar/baz/", "**/bar/*" },
+   { 1, "deep/foo/bar/baz/", "**/bar/**" },
+   { 0, "deep/foo/bar", "**/bar/*" },
+   { 1, "deep/foo/bar/", "**/bar/**" },
+   { 0, "foo/bar/baz", "**/bar**" },
+   { 1, "foo/bar/baz/x", "*/bar/**" },
+   { 0, "deep/foo/bar/baz/x", "*/bar/**" },
+   { 1, "deep/foo/bar/baz/x", "**/bar/*/*" },
+
+   /* Various additional tests */
+   { 0, "acrt", "a[c-c]st" },
+   { 1, "acrt", "a[c-c]rt" },
+   { 0, "]", "[!]-]" },
+   { 1, "a", "[!]-]" },
+   { 0, "", "\\" },
+   { 0, "\\", "\\" },
+   { 0, "XXX/\\", "*/\\" },
+   { 1, "XXX/\\", "*/" },
+   { 1, "foo", "foo" },
+   { 1, "@foo", "@foo" },
+   { 0, "foo", "@foo" },
+   { 1, "[ab]", "\\[ab]" },
+   { 1, "[ab]", "[[]ab]" },
+

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-08 Thread Junio C Hamano 
Ævar Arnfjörð Bjarmason  writes:

> I agree that there should be some prerequisite to skip this on Windows
> by default since 6 minutes is clearly excessive (although I think you'll
> find it runs a bit faster in the branch above), but that should be
> something like:
>
> test_lazy_prereq EXPENSIVE_ON_WINDOWS '
> test -n "$GIT_TEST_LONG" || test_have_prereq !MINGW,!CYGWIN
> '
>
> As the long runtime is not inherent to the test, but to excessive
> slowness caused by certain operations on certain platforms, or maybe it
> should be EXPENSIVE_ON_SLOW_FS or EXPENSIVE_IF_FORKING_IS_SLOW or if
> we'd like to generalize it.

We already do skip overly long tests everywhere unless explicitly
asked to run them, and the above sounds like a good way to go.

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-08 Thread Johannes Schindelin 
Hi Ævar,

On Sat, 6 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> Can you please provide me with the output of the test under -v -x -d
> from github.com:avar/git.git wildmatch-refactor-8 branch?

With -v -x -i:

-- snip --
[...]
expecting success:
printf '%s' '?a?b' >expect &&
git --glob-pathspecs ls-files -z -- '\??\?b' 
>actual.raw 2>actual.err &&

tr -d '\0' actual &&
>expect.err &&
test_cmp expect.err actual.err &&
test_cmp expect actual

++ printf %s '?a?b'
++ git --glob-pathspecs ls-files -z -- '\??\?b'
+ test_eval_ret_=128
+ want_trace
+ test t = t
+ test t = t
+ set +x
error: last command exited with $?=128
not ok 734 - wildmatch(ls): match '\??\?b' '?a?b'
#
#   printf '%s' '?a?b' >expect &&
#   git --glob-pathspecs ls-files -z
#   -- '\??\?b' >actual.raw
#   2>actual.err &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#

real2m9.127s
user0m10.026s
sys 1m0.617s
-- snap --

and

-- snip --
$ cat ./trash\ directory.t3070-wildmatch/actual.err
fatal: Invalid path '/??': No such file or directory
-- snap --

As to the speed:

-- snip --
# still have 144 known breakage(s)
# failed 28 among remaining 1746 test(s)
1..1890

real5m55.162s
user0m26.396s
sys 2m34.152s
-- snap --

... seems to be in the same ballpark. You are just leaning way too heavily
on Unix shell scripting.

FWIW the breakages are:

-- snip --
not ok 734 - wildmatch(ls): match '\??\?b' '?a?b'
#
#   printf '%s' '?a?b' >expect &&
#   git --glob-pathspecs ls-files -z
#   -- '\??\?b' >actual.raw
#   2>actual.err &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#
ok 735 - iwildmatch: match '?a?b' '\??\?b'
not ok 736 - iwildmatch(ls): match '\??\?b' '?a?b'
#
#   printf '%s' '?a?b' >expect &&
#   git --glob-pathspecs
#   --icase-pathspecs ls-files -z --
#   '\??\?b' >actual.raw 2>actual.err
#   &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#
ok 737 - pathmatch: match '?a?b' '\??\?b'
not ok 738 - pathmatch(ls): match '\??\?b' '?a?b'
#
#   printf '%s' '?a?b' >expect &&
#   git ls-files -z -- '\??\?b'
#   >actual.raw 2>actual.err &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#
ok 739 - ipathmatch: match '?a?b' '\??\?b'
not ok 740 - ipathmatch(ls): match '\??\?b' '?a?b'
#
#   printf '%s' '?a?b' >expect &&
#   git --icase-pathspecs ls-files -z
#   -- '\??\?b' >actual.raw
#   2>actual.err &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#
ok 741 - cleanup after previous file test
ok 742 - setup wildtest file test for abc
ok 743 - wildmatch: match 'abc' '\a\b\c'
not ok 744 - wildmatch(ls): match '\a\b\c' 'abc'
#
#   printf '%s' 'abc' >expect &&
#   git --glob-pathspecs ls-files -z
#   -- '\a\b\c' >actual.raw
#   2>actual.err &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#
ok 745 - iwildmatch: match 'abc' '\a\b\c'
not ok 746 - iwildmatch(ls): match '\a\b\c' 'abc'
#
#   printf '%s' 'abc' >expect &&
#   git --glob-pathspecs
#   --icase-pathspecs ls-files -z --
#   '\a\b\c' >actual.raw 2>actual.err
#   &&
#
#   tr -d '\0' actual &&
#   >expect.err &&
#   test_cmp expect.err actual.err &&
#   test_cmp expect actual
#
ok 747 - pathmatch: match 'abc' '\a\b\c'
not ok 748 - pathmatch(ls): match

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-08 Thread Johannes Schindelin 
Hi Duy,

On Sun, 7 Jan 2018, Duy Nguyen wrote:

> On Sat, Jan 6, 2018 at 7:51 PM, Johannes Schindelin
>  wrote:
> > Nobody likes to run tests that take too
> > long. And look at this:
> >
> > ...
> > ok 1511 - ipathmatch: match 'Z' '[Z-y]'
> > ok 1512 - ipathmatch(ls): match '[Z-y]' 'Z'
> > # still have 84 known breakage(s)
> > # failed 52 among remaining 1428 test(s)
> > 1..1512
> >
> > real5m51.432s
> > user0m33.986s
> > sys 2m13.162s
> >
> > Yep. It takes *over eight minutes*.
> 
> I suppose this is because the sheer number of test cases adds a lot of
> shell overhead on Windows. I wonder if it's better to rewrite this
> test in C instead. We start to do some more unit testing here and
> there and kind of abuse the sh-based test framework for this. Having a
> proper unit test framework would be good anyway since it's sometimes
> hard to create a specific scenario with high level commands.

I agree that it would make a ton of sense to use a proper, portable test
framework written in pure, portable C.

However, this ship has long sailed, hasn't it?

Of course, we do have useful stuff in t/helper/. And we have precedent for
more sensible bulk testing that does not require sh to generate or provide
lists of test data, see e.g. the basename/dirname tests. And we could
organize t/helper/ better, including a refactoring to have a single binary
rather than tons and tons of binaries that all link to libgit.a and
take a lot of space (even with LZMA compression, the current test
artifacts take about 90 megabyte!).

If I had the time I would write this up as a valuable GSoC project. Not
that Junio cares. But I do.

Ciao,
Dscho

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-06 Thread Duy Nguyen 
On Sat, Jan 6, 2018 at 7:51 PM, Johannes Schindelin
 wrote:
> Nobody likes to run tests that take too
> long. And look at this:
>
> ...
> ok 1511 - ipathmatch: match 'Z' '[Z-y]'
> ok 1512 - ipathmatch(ls): match '[Z-y]' 'Z'
> # still have 84 known breakage(s)
> # failed 52 among remaining 1428 test(s)
> 1..1512
>
> real5m51.432s
> user0m33.986s
> sys 2m13.162s
>
> Yep. It takes *over eight minutes*.

I suppose this is because the sheer number of test cases adds a lot of
shell overhead on Windows. I wonder if it's better to rewrite this
test in C instead. We start to do some more unit testing here and
there and kind of abuse the sh-based test framework for this. Having a
proper unit test framework would be good anyway since it's sometimes
hard to create a specific scenario with high level commands.
-- 
Duy

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-06 Thread Johannes Schindelin 
Hi Ævar,

On Sat, 6 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> As I explained in 20180105221222.28867-1-ava...@gmail.com the actual
> benefit of this test is that as much as possible is tested
> *somewhere*.

And what I am trying to get across is that your tests are excessive. I do
not see the benefit in running such an excessive, overzealous test.

You say that Linux will not overnight stop supporting colons in filenames,
which kind of misses the point that the question is more about new
platforms needing Git support with a *filesystem* that simply does not
meet one of your many tests' expectations.

But what is equally true is that few changes are to be expected in the
wildmatch code. And you know as well as I that it is possible to come up
with a careful, small set of test cases that will most likely identify
breakages.

After all, when there is a bug in some patch to the wildmatch machinery,
you do not need 512 test cases to fail. A single failing test case is
definitely enough.

I'll test the branch you indicated on Monday, just because you put effort
into it.

But please note that it does not change the fact that an effective test
suite requires a balance between the need to run fast (if *every* aspect
of Git was tested as extensively as your current t3070, even on Linux it
would take hours and hours to finish the test suite, making it stupidly
expensive for everybody to run) and the need to catch regressions.

It is a well-established rule in effective DevOps that you want lean test
suites. Actionable test suites. It is enough for *one* test to fail. When
a test fails, it should be as easy to understand as possible what is going
wrong. If there are no regressions, the test suite should be passing
*fast*.

On Windows, it takes 70-90 minutes *on a fast* machine. That is a huge
failure on our part: this is the test suite we designed "to be portable".
All I am trying to do here is to prevent even more damage. I will
appreciate any assistance to that end. I understand that most developers
on this list simply don't care. And that's just too bad, because we could
do a lot better. And we should, too.

Ciao,
Johannes

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-06 Thread Ævar Arnfjörð Bjarmason 

On Sat, Jan 06 2018, Johannes Schindelin jotted:

> Hi Junio,
>
> On Fri, 5 Jan 2018, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason   writes:
>>
>> > Skip the newly added file creation tests on Windows proper, these
>> > already work under Cygwin, but as that involves a significant
>> > emulation layer the results are different under Windows proper with
>> > MinGW.
>> > ...
>> > diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
>> > index f606f91acb..50a53e7a62 100755
>> > --- a/t/t3070-wildmatch.sh
>> > +++ b/t/t3070-wildmatch.sh
>> > @@ -7,6 +7,14 @@ test_description='wildmatch tests'
>> >  create_test_file() {
>> >file=$1
>> >
>> > +  # These tests limp along under Cygwin, but the failures on
>> > +  # native Windows are still too many. Skip file tests there
>> > +  # until they're sorted out.
>> > +  if test_have_prereq MINGW
>> > +  then
>> > +  return 1
>> > +  fi
>> > +
>>
>> That looks to be a nuclear option.
>
> Indeed:
>
>   # still have 84 known breakage(s)
>   # failed 52 among remaining 1428 test(s)
>   1..1512
>
> Let's just switch it off completely because 52 of those 1512 test cases
> break only on Windows? Pretty heavy-handed.

Can you please provide me with the output of the test under -v -x -d
from github.com:avar/git.git wildmatch-refactor-8 branch?

It has some improvements that should make things a bit faster for you,
but also most of the logic is now in test_expect_success so -x is more
helpful.

I should then be able to fix these 52 remaining cases (and at least 1 of
them should be fixed already).

> But do read on.
>
>> For now it may be suffice, but somehow it feels that it goes directly
>> against Dscho's wish to treat (or pretend to treat) Windows as
>> first-class citizens and/or just like other platforms.
>
> It not only goes against my wish to treat Windows as a normal citizen
> instead of like Rey's parents, it also goes against my wish to have a
> focused and meaningful test suite. Nobody likes to run tests that take too
> long. And look at this:
>
>   ...
>   ok 1511 - ipathmatch: match 'Z' '[Z-y]'
>   ok 1512 - ipathmatch(ls): match '[Z-y]' 'Z'
>   # still have 84 known breakage(s)
>   # failed 52 among remaining 1428 test(s)
>   1..1512
>
>   real5m51.432s
>   user0m33.986s
>   sys 2m13.162s
>
> Yep. It takes *over eight minutes*.
>
> And this is a *fast* machine.
>
> Why? Because of the completely overboard testing of all kinds of
> *potential* breakages *at some point* in the future.
>
> I understand that Ævar wants to increase test coverage. I am sympathetic
> to this cause.
>
> But I completely disagree that increasing the test coverage beyond reason
> is a good thing. Tests *can* take too long, and they do, in practice, and
> the results are always problematic: every developer who has to deal with
> test suites that take too long... does not run them. As simple as that.
> And then you have *zero* test coverage. Pretty stupid, eh? And contrary to
> your intentions, too.
>
> So yes, I think that it has been proven beyond any doubt that t3070 takes
> *waay* too long on Windows, for dubitable benefit.
>
> I could see a reasonable compromise where
>
> - an extensive test matrix is hidden behind an EXPENSIVE_WILDMATCH prereq,
>
> - the test matrix would be written in a much more understandable way, i.e.
>   using English words rather than "1"s. If need be, there could be blocks
>   ("test this with case-sensitive matching", "skip case-sensitive matching",
>   you get the idea),

I take your point with the readability of some of this stuff, and will
get around to fixing that before the next submission.

> - these magic skippings of certain test cases (where the (non-traced)
>   `create_test_file()` function returns 1 for certain things) still would
>   need to go away, in favor probably of prereqs and/or blocks where flags
>   are set accordingly in a preamble,

This is a lot of work for dubious benefit. I'm not going to try to
maintain some exhaustive mapping that's potentially going to be a hash
4-levels deep of:

os.os_version.fs.vs_version

And that's before we'd get to the potential 6-level crazyness of:

os.os_version.os_flags.fs.vs_version.fs_flags

It's way easier and more portable (even to OSs or FSs nobody's invented
yet) to just test whether a file can be created, and if not skip the
test.

As I explained in 20180105221222.28867-1-ava...@gmail.com the actual
benefit of this test is that as much as possible is tested
*somewhere*. There's no reason to suspect that e.g. Linux will overnight
make the character ":" illegal in filenames and we'll be the only ones
who notice it.

Since we don't treat ":" or any other character differently for the
purposes of glob matching on Windows, but *do* treat the raw glob
matching differently than calling wildmatch() directly, as the current
tests do, I really don't see what the point of this exercises would be.

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-06 Thread Johannes Schindelin 
Hi Junio,

On Fri, 5 Jan 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason   writes:
> 
> > Skip the newly added file creation tests on Windows proper, these
> > already work under Cygwin, but as that involves a significant
> > emulation layer the results are different under Windows proper with
> > MinGW.
> > ...
> > diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> > index f606f91acb..50a53e7a62 100755
> > --- a/t/t3070-wildmatch.sh
> > +++ b/t/t3070-wildmatch.sh
> > @@ -7,6 +7,14 @@ test_description='wildmatch tests'
> >  create_test_file() {
> > file=$1
> >  
> > +   # These tests limp along under Cygwin, but the failures on
> > +   # native Windows are still too many. Skip file tests there
> > +   # until they're sorted out.
> > +   if test_have_prereq MINGW
> > +   then
> > +   return 1
> > +   fi
> > +
> 
> That looks to be a nuclear option.

Indeed:

# still have 84 known breakage(s)
# failed 52 among remaining 1428 test(s)
1..1512

Let's just switch it off completely because 52 of those 1512 test cases
break only on Windows? Pretty heavy-handed.

But do read on.

> For now it may be suffice, but somehow it feels that it goes directly
> against Dscho's wish to treat (or pretend to treat) Windows as
> first-class citizens and/or just like other platforms.

It not only goes against my wish to treat Windows as a normal citizen
instead of like Rey's parents, it also goes against my wish to have a
focused and meaningful test suite. Nobody likes to run tests that take too
long. And look at this:

...
ok 1511 - ipathmatch: match 'Z' '[Z-y]'
ok 1512 - ipathmatch(ls): match '[Z-y]' 'Z'
# still have 84 known breakage(s)
# failed 52 among remaining 1428 test(s)
1..1512

real5m51.432s
user0m33.986s
sys 2m13.162s

Yep. It takes *over eight minutes*.

And this is a *fast* machine.

Why? Because of the completely overboard testing of all kinds of
*potential* breakages *at some point* in the future.

I understand that Ævar wants to increase test coverage. I am sympathetic
to this cause.

But I completely disagree that increasing the test coverage beyond reason
is a good thing. Tests *can* take too long, and they do, in practice, and
the results are always problematic: every developer who has to deal with
test suites that take too long... does not run them. As simple as that.
And then you have *zero* test coverage. Pretty stupid, eh? And contrary to
your intentions, too.

So yes, I think that it has been proven beyond any doubt that t3070 takes
*waay* too long on Windows, for dubitable benefit.

I could see a reasonable compromise where

- an extensive test matrix is hidden behind an EXPENSIVE_WILDMATCH prereq,

- the test matrix would be written in a much more understandable way, i.e.
  using English words rather than "1"s. If need be, there could be blocks
  ("test this with case-sensitive matching", "skip case-sensitive matching",
  you get the idea),

- these magic skippings of certain test cases (where the (non-traced)
  `create_test_file()` function returns 1 for certain things) still would
  need to go away, in favor probably of prereqs and/or blocks where flags
  are set accordingly in a preamble,

- by default, i.e. without the EXPENSIVE_WILDMATCH prereq, it should test
  a *tiny* subset that is indicative of the most likely bugs.

As it is, I like 8/7 in the present form for all the wrong reasons: it
protects me from the damage a full t3070 would do to me.

Ciao,
Dscho

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

2018-01-05 Thread Junio C Hamano 
Ævar Arnfjörð Bjarmason   writes:

> Skip the newly added file creation tests on Windows proper, these
> already work under Cygwin, but as that involves a significant
> emulation layer the results are different under Windows proper with
> MinGW.
> ...
> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> index f606f91acb..50a53e7a62 100755
> --- a/t/t3070-wildmatch.sh
> +++ b/t/t3070-wildmatch.sh
> @@ -7,6 +7,14 @@ test_description='wildmatch tests'
>  create_test_file() {
>   file=$1
>  
> + # These tests limp along under Cygwin, but the failures on
> + # native Windows are still too many. Skip file tests there
> + # until they're sorted out.
> + if test_have_prereq MINGW
> + then
> + return 1
> + fi
> +

That looks to be a nuclear option.  For now it may be suffice, but
somehow it feels that it goes directly against Dscho's wish to treat
(or pretend to treat) Windows as first-class citizens and/or just
like other platforms.