-
-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct subtest placement #50
Conversation
Just need some time to sit down with calm and understand what's changed.
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. |
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:
But in fact we can have things like:
and we have to keep a record that 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:
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 Having a major version of this would be very nice. |
I can also simplify some regexes a bit, if you want. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -58,7 +58,7 @@ private Patterns() { // hidden as this is a final utility class | |||
/** | |||
* TAP Test Result Regex. | |||
*/ | |||
static final String REGEX_TEST_RESULT = "((\\s|\\t)*)?(ok|not ok)\\s*(\\d*)\\s*([^#]*)?\\s*" | |||
static final String REGEX_TEST_RESULT = "(\\s*)(ok|not ok)\\s*(\\d*)\\s*([^#]*)?\\s*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we should remove tabs here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this minor issue is present in other regexes too, but I didn't fix it, because not sure if wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, *
takes exactly zero or more elements, so you can omit redundant ?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
import org.tap4j.model.TestResult; | ||
import org.tap4j.model.TestSet; | ||
import org.tap4j.model.Text; | ||
import org.tap4j.model.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll update it soon.
state.attachedToParent = true; | ||
} | ||
} | ||
if (indentation - prevIndent > 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Merged! Preparing a release tonight. Thanks a lot for the great work and patience in getting it merged @Altai-man |
@kinow thanks. |
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? |
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 |
Not sure about that. I think that you can take e.g. Thanks a lot for the release. |
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.