Add support for TAP version 12. #26
Conversation
fmartinsons
force-pushed
the
make-version-not-mandatory
branch
3 times, most recently
from
b530563
to
fe59719
Compare
tap2junit/tap13.py
Outdated
| non_empty_lines = [ | ||
| line for line in source.getvalue().splitlines() if len(line.strip()) > 0 | ||
| ] |
There was a problem hiding this comment.
| non_empty_lines = [ | |
| line for line in source.getvalue().splitlines() if len(line.strip()) > 0 | |
| ] | |
| non_empty_lines = [line for line in source.getvalue().splitlines() if line] |
There was a problem hiding this comment.
This will not cover line with only spaces in it so I'll just suppress the len usage.
There was a problem hiding this comment.
For Node.js needs we only really to support TAP 13 which has been around for a long time and is widely supported. http://testanything.org/producers.html How would it help the Node.js community to add this complexity and support burden?
tap2junit/tap13.py
Outdated
| non_empty_lines = [ | ||
| line for line in source.getvalue().splitlines() if len(line.strip()) > 0 | ||
| ] | ||
| m = RE_VERSION.match(non_empty_lines[0]) |
There was a problem hiding this comment.
Single letter variable names died in the late 1970's. Please use self-documenting variable names.
There was a problem hiding this comment.
Sure I'll use another names, I just was tricked by already single letter variable usage (which I'll replace also)
tap2junit/tap13.py
Outdated
| m = RE_VERSION.match(non_empty_lines[0]) | ||
| if m: | ||
| # It is an error if version is anything below 13 (so not an int is an error) | ||
| v = int(m.groupdict()["version"]) |
There was a problem hiding this comment.
| v = int(m.groupdict()["version"]) | |
| version = int(m.groupdict()["version"]) |
test/fixtures/test-plan-at-end.tap
Outdated
| # My test 2 comments | ||
| ok 3 /MyTest/3 | ||
| not ok 4 /MyTest/3 | ||
| of /MyTest/4 |
There was a problem hiding this comment.
Is this a typo? (Same question for the other tap file).
There was a problem hiding this comment.
Oh my, of course it is a typo. Thanks
The link you provided shows there is still several TAP produces which doensn't use TAP 13 (I count 7 of them where it is explicitely mentionned that produces TAP 12 but I'm sure there are more if we look on details on each projects). I don't know how to measure amount of producers using TAP 12 vs 13 and I don't know to Node.js community, but considering what I think to be a minimal support burden for TAP 12 in tap2junit (around 90% of the code parser is common), I think it may a great help for people which use tap2junit (which I understand to be a generic tool not tied to Node.js) |
fmartinsons
force-pushed
the
make-version-not-mandatory
branch
from
fc59623
to
81814a5
Compare
|
This merge requests hangs for 3 weeks now, is anything wrong with it ? |
fmartinsons
force-pushed
the
make-version-not-mandatory
branch
from
81814a5
to
cc80bfc
Compare
|
Ok thanks, I amended my MR to reflect the support of version 12 in README file. |
|
@fmartinsons Please rebase your PR because I would like to put out a new release. |
Oh, I must admit I totally forgot this PR since it lasts for a long time. Are you sure about rebasing this ? Did you still plan to merge this one ? Anyway, I must redo my dev env for this project since I wiped out my PC several months ago. If you think this dev is still accurate, I'll do the job in the coming days. |
This is the consequences of two things taken from the TAP specification document: https://testanything.org/tap-specification.html - version line must be the first line and is only required for TAP version 13, every version explicitly set below 13 is an error. - YAML management is only specified for version 13 Add test scenarios for version line missing and test plan at the end (this is specified that the plan could be at the beginning of output or at the end) Signed-off-by: Frederic Martinsons <[email protected]>
fmartinsons
force-pushed
the
make-version-not-mandatory
branch
from
cc80bfc
to
a30b431
Compare
|
Ok I rebase, but I'll need to rework the parse function as it is now too complex. |
Signed-off-by: Frederic Martinsons <[email protected]>
fmartinsons
force-pushed
the
make-version-not-mandatory
branch
from
e009f49
to
e23cd26
Compare
|
Done after reducing a little the complexity of |
| for line in source: | ||
| if not seek_version and not in_yaml and RE_VERSION.match(line): | ||
| if not version12 and not seek_version and RE_VERSION.match(line): |
There was a problem hiding this comment.
did you test what happens when inside a yaml and the yaml contains a version line? thats why this tested fornot in_yaml
There was a problem hiding this comment.
@cclauss @fmartinsons this has broken #31
There was a problem hiding this comment.
Oh my, this was a mistake.
This flag may have removed when I resolved conflict on my branch.
I open #35 to fix that. Sorry
There was a problem hiding this comment.
thanks, I found that out when working on #34
This is especially usefull when a version line is contained inside a yaml block Signed-off-by: Frederic Martinsons <[email protected]>
This is especially usefull when a version line is contained inside a yaml block Signed-off-by: Frederic Martinsons <[email protected]>
This is especially usefull when a version line is contained inside a yaml block Signed-off-by: Frederic Martinsons <[email protected]>
This is especially usefull when a version line is contained inside a yaml block Signed-off-by: Frederic Martinsons <[email protected]>
This is the consequences of two things taken from the TAP specification
document: https://testanything.org/tap-specification.html
version 13, every version explicitly set below 13 is an error.
Add test scenarios for version line missing and test plan at the end
(this is specified that the plan could be at the beginning of output
or at the end)
Signed-off-by: Frederic Martinsons [email protected]