Correct subtest placement

[Altai-man]

This PR enables subtest parsing with appending it to correct TestResult node, discussed at #48.
It passes all tests with minor tweaks to the suite regarding cases where test data was incorrect(judging by Perl-style subtests, of course). It closes the issue.

On the other hand, I can see that breakage of backward compatibility can harm a lot, so we can probably enable it by a flag or the PR can be just rejected.

[coveralls]

Coverage increased (+0.06%) to 94.347% when pulling b2524e3 on Altai-man:master into 5630f68 on tupilabs:master.

[kinow]

Just need some time to sit down with calm and understand what's changed.

so we can probably enable it by a flag or the PR can be just rejected.

Definitely not rejected. From what I remember, I implemented contrary to what other tools were using, for no clear reason. Keen to either a major version, or a flag as you suggested :-)

Thanks a ton for the pull request. Hopefully I will be able to use the momentum to work a bit on the tap plugin for Jenkins as well.

[Altai-man]

Just need some time to sit down with calm and understand what's changed.

Well, I am not a super hacker myself, so it took some time to wrap my head around that too. :)

I'll try to explain my code a bit...

The main issue with current algorithm is that it assumes gradual "going down one level per step", as in:

ok 1
    ok 2
        ok 3

But in fact we can have things like:

ok 1
        ok 2.1.1
    ok 2.1
ok 2

and we have to keep a record that 2.1.1 belongs to 2.1, which belongs to 2, and don't leave anything behind.

So we have two stack of directions, one is left-to-right(your usual state, that keeps state when we do a usual increase(one level, 4 spaces), second one is right-to-left(when we have a "jump" for more than one space, it means it is a subtest of some other test, but we don't have "the future one" to attach it yet). Hence we're saving two variants of states.

The second issue is that we need a proper state placement: when the indentation decreases, we have a choice: either we already have this state in our stack(so we're just using that) or we don't yet encountered this state, so we need to create it, for example:

ok
    # maybe "ok1" is here
        ok
    ok # <- this ok, if ok1 was there, then we already created such a state, but if not, we need a new one
...

Both stacks are meant to be empty at certain point of time, for "usual" states: when the indentation of previously known subtest decreases, for "weird ones": when we encounter a testResult with zero indentation, then we just attach all things that were hold on like a tree.

I have changed Tap13Representer to match subtests before and added some fixes to regexes(though more can be done).

I am not very satisfied with the code myself, so if you can provide some counter-example that won't be parse-able(yet technically correct), I would be glad to look into it, though I couldn't.

Important note: also, if I remember correctly this PR breaks parsing when subtestEnabled is set to false and subtest occurs. I am not sure why do we want such flag, but we probably want to be smarter with that anyway. Maybe just skip any strings with non-basic indentation?

Having a major version of this would be very nice.

[Altai-man]

I can also simplify some regexes a bit, if you want. :)

[kinow]

Just need a bit more of time to re-read the part with the two stacks, and maybe debug a bit to see it in action. But everything else looks good. Minor comments. Thought it would be better to leave a partial review than waiting until I've finished. Change looks great and very promising! Looking forward to merging and releasing it soon! Ta

[kinow]

Any reason why we should remove tabs here too?

[Altai-man]

The thing is \s includes \t, so it's redundant to use alternating matching between them. See https://docs.oracle.com/javase/tutorial/essential/regex/pre_char_classes.html

[Altai-man]

And this minor issue is present in other regexes too, but I didn't fix it, because not sure if wanted.

[kinow]

Just past midnight here, so the new day barely started and I can proudly say Today-I-Learned.

Had no idea they were redundant. I've been using both in Perl, shell, java... might have a few places to fix besides tap4j.

This change is definitely wanted. But we can fix in another issue if that's easier.

Thanks!!!

[Altai-man]

Also, * takes exactly zero or more elements, so you can omit redundant ?.

[kinow]

Right. I think you are talking about this part:

([^#]*)?

I thought that it would resolve [^#]*as zero or more of [^#] in a group ()?. And so the group was receiving the ?, and thus would appear zero or at most once?

[Altai-man]

I wrote about e.g. ((\\s|\\t)*)?(putting \t symbol aside) - in this case, ? is redundant.
We capture "zero or more space characters" for "zero or one time". It could be meaningful with (.+)?("one or more any character" could or could not occur), but it's roughly a wordy equivalent of just (.*).
That's what I meant.

[kinow]

Aahh! Sure. You fixed that one already, but I agree for the remaining regular expressions. Gonna start thinking a bit more when writing regexes now :)

[kinow]

I can re-enter the imports later, but if you could change to specific imports that'd be simpler. I know it sounds nit-picking, but at work we actually asked another dev that works on an IDE that was doing that. While doing a refactoring of old code, even her IDE would fail to identify the classes that it had to use, as we had a few duplications 😢

[Altai-man]

Sure, I'll update it soon.

[kinow]

Could we use if (indentation - prevIndent > 0) instead? If I understand it right, we are changing the state only when we find a statement with 4 spaces more than the previous statement? Not sure if that's defined somewhere, but I suspect there might be tools/devs doing 2 spaces, or 3...

[Altai-man]

Not sure if that's defined somewhere

Well, specification is not the strong point of TAP at all... I see at least a long conversation at TestAnything/Specification#2. But the thing is, all origin sources of truth(e.g. Perl) are using exact four spaces, not de jure, but de facto standard. It is up to you, of course, but I think If someone is doing weird indentation, it is theirs thing to patch.

If I understand it right, we are changing the state only when we find a statement with 4 spaces more than the previous statement?

Yes, We cannot use > 0 here. This check is needed to differentiate between those cases:

ok
    ok # 4 spaces, all is nice
ok

and

ok
            ok # 12 spaces, need to push in subtests stack
        ok
    ok
ok

That's why we are checking exact more than 4 spaces.

[kinow]

Merged! Preparing a release tonight. Thanks a lot for the great work and patience in getting it merged @Altai-man

[Altai-man]

@kinow thanks.
Do you plan to do anything on regard to inability to parse texts that include subtests, when subtests are disabled?

[kinow]

Haven't thought about it. Lately I've been spending more time on R & work-related projects (alas current work has no TAP producer/consumer). But keen to spend some time working on any issues related to TAP in the next 2 months (weather is terrible here around Oceania right now). Is there already a ticket for that?

[kinow]

And 4.3. released by the way. Servers syncing, jar's should be available in the next minutes/hour. And even though we just released it, of course we can release a 4.3.1, 4.4, any time from now too. RERO

[Altai-man]

Is there already a ticket for that?

Not sure about that. I think that you can take e.g. TestSubtestOrder, replace makeTap13YamlConsumer with makeTap13Consumer and the test will fail with duplicated tap plan found. In my opinion it is really a design-wise decision: do you want to properly throw an exception when subtests are disabled and then some indentation occurs, or just ignore anything that does not match base indentation.

Thanks a lot for the release.