Add fixture tests for gix-url

[niklaswimmer]

As part of #933 I think it would be a good start to add fixture tests for gix-url, to verify that our behavior matches what Git does. I basically adjusted what I found in the gix-glob test suite.

• add make_baseline.sh
• add test that interacts with the fixture test suite for all generic urls
  • parse Gits output
• do not fail fast and show all mismatches
• fixture tests for platform specific url parsing? (@Byron I need your input on this, I am not sure whether it is possible with the current test suite)

[Byron]

I am loving the new baseline test, thanks so much!
For suggestions on how to do the parsing, gix-attributes probably could serve as a more recent example, maybe gix-pathspec is easier to read though and the cleaner of the two.

I am very much looking forward to seeing this PR come to completion, it's a great start!

[niklaswimmer]

Up until now I was able to just replicate the structure from other fixture tests, but frankly speaking I have no idea how the whole machinery works. So I am unsure what to do about that CI failure - I probably missed something.

Btw, are there any resources on the whole fixture testing machinery? I think I saw none

[niklaswimmer]

I just now realize an interesting side effect of using a HashMap to store the test cases. Because the insertion order is not preserved I get presented a new error every time I run the test :)

[Byron]

Btw, are there any resources on the whole fixture testing machinery? I think I saw none

No, but for what it's worth, you seem to have picked up the idea perfectly!
I did an initial refactor to bring it more inline with what's already there, while making it easier to see where we are with the baseline itself. For that it was made deterministic and now gathers some information about the whole run.

This will allow you to pick the lowest-hanging fruit, pull out the URL into a separate test, and work on that separately if it helps. That way the baseline merely provides a guidance for what kind of URL you want to implement/make work next, until everything parses correctly. After all, right now it prints a long report and doesn't fail immediately, which makes it less useful as a test to work with. You might also find it useful to tune the report so it helps finding what's wrong more easily.

The next step I'd chose is to pick a failing URL, put it into a separate test whose expectation matches what git produces (just hardcode that), to be able to conveniently work on getting it to pass. This should then increase the number of correct answers in the baseline. Then rinse and repeat.

I hope that helps.

PS: It's certainly fine to #[ignore] the baseline test while it's failing so CI can be green when pushing.

[niklaswimmer]

The next step I'd chose is to pick a failing URL, put it into a separate test whose expectation matches what git produces (just hardcode that), to be able to conveniently work on getting it to pass.

I think I will just rewrite the parsing now instead of trying to fix failures individually. I have the feeling doing so would make the parsing code even more complex. I also have a good idea of all the edge cases by now - so a rewrite should make most cases pass.

[niklaswimmer]

Ehhh, I did a little something niklaswimmer#1. I did not push it here because then I would potentially have to deal with reverts or removing the commit from existence if this is something you do not want.

Let me know if find it as fancy as I do 🙈

[Byron]

Ehhh, I did a little something niklaswimmer#1. I did not push it here because then I would potentially have to deal with reverts or removing the commit from existence if this is something you do not want.

I think you really want Stacked Git, which makes patch-queues easy. I use it all the time (and it's powered by gix, too :D).

Let me know if find it as fancy as I do 🙈

Taking a look now :).

[niklaswimmer]

I was also thinking, many of the fixture tests follow the very same simple cycle of

1. run fixture script / extract archive / do whatever scripted_fixture_* does
2. parse input file into some struct
3. run some asserts on the input
4. output the results

If number four is taken care of by libtest_mimic I think it would be quite possible to create a helper function that just takes the script's name, a splitting pattern, a function that turns some &BStr into a struct and another function that gets passed the struct and is supposed to call a bunch of assert macros.

[Byron]

I liked the minimal invasiveness of the current implementation, but noticed that it seems to interleave the panic text/captured output. It's strange that his happens, I would have expected this to be sorted by a test-runner.

If number four is taken care of by libtest_mimic I think it would be quite possible to create a helper function that just takes the script's name, a splitting pattern, a function that turns some &BStr into a struct and another function that gets passed the struct and is supposed to call a bunch of assert macros.

This sounds like refactoring, but also a little bit like bike-shedding as to my mind the baseline code is pretty straightforward with just the right amount of abstraction. But, since you are working with it, I think you should be most comfortable and I trust your intuition to do what's right.

[niklaswimmer]

It's strange that his happens, I would have expected this to be sorted by a test-runner.

I think the problem is that even though we use catch_unwind, panic messages are still printed normally. This bothered me already before I introduced libtest_mimic and I am unsure how to change that (changing the panic hook does not work, because that is global, and a test harness may run multiple tests in different threads leading to even weirder output).

I think this may be relevant? https://docs.rs/libtest-mimic/latest/libtest_mimic/#known-limitations-and-differences-to-the-official-test-harness

[niklaswimmer]

This sounds like refactoring, but also a little bit like bike-shedding as to my mind the baseline code is pretty straightforward with just the right amount of abstraction.

Jeah it was just an idea that could be done to make writing new fixture tests faster. Not something particular relevant for this PR, sorry if I am mixing things up a bit..

[Byron]

I think this may be relevant? https://docs.rs/libtest-mimic/latest/libtest_mimic/#known-limitations-and-differences-to-the-official-test-harness

That seems very much like it. I never tried to alter the panic handler, but would have thought the library handles this.
This sours it a bit for me and I will most certainly keep things as is, but there nothing speaking against using it here in the interim - it is useful nonetheless even though the interleaving would really bother me.

Before merging, I will probably remove it if alone for the fact that it adds a lot of generated tests to the suite that inflate the count ultimately adding more noise.

[niklaswimmer]

it is useful nonetheless even though the interleaving would really bother me

Fixed that one. I am somewhat ambivalent about the increasing test number - I do not really mind it; but I leave that decision up to you when the time comes.

For now I should probably stop messing with the test suite and instead finally work towards making them pass too :)

[Byron]

I am preparing to merge what's here because particularly the improvements to the makefile and justfile shouldn't have to wait. All baseline tests have been turned into ignored tests while most of them fail so the test-suite can run like before.

For changing the implementation, I kindly ask you to do that in a follow-up PR when there is time.

Thanks a lot for all your work thus far :)

[Byron]

Closing as this PR was merged (but GitHub can't tell anymore as I changed my most recent commit locally).

[niklaswimmer]

Understood, the next PR is already in the making :)
Thanks for your kind feedback along the way and for your fast responses, working on this really was a pleasure.