Skip to content
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

Wrong YAML block in TAP report #109

Closed
jeberger opened this issue Feb 22, 2019 · 1 comment · Fixed by #110
Closed

Wrong YAML block in TAP report #109

jeberger opened this issue Feb 22, 2019 · 1 comment · Fixed by #110
Assignees

Comments

@jeberger
Copy link
Contributor

YAML blocks in TAP reports may be wrong if one of the fields spans more than one line, which is often the case for stack traces. For example:

not ok 5 Voice > State
  ---
  message: "Test failed"
  severity: failed
  actual: 1
  expected: 2
  stack:     at function1 (/path/to/test.js:11:3)
    at function2 (/path/to/test.js:22:3)
  ...

The stack trace is not valid YAML. It should follow the YAML line folding rules.

trentmwillis pushed a commit that referenced this issue May 8, 2019
* Reporter: Fix YAML output in TAP reporter

When a message or stack trace contains double quotes, or when the stack
trace contains more than one line, we escape the values.

Fixes #109

* Reporter: Fix style issues reported by Travis

* Reporter: Fix issue when message is undefined.

* Reporter: Fix `actual` and `expected` fields in YAML output.

* Reporter: Fix unit tests.
@Krinkle Krinkle reopened this Sep 7, 2020
@Krinkle Krinkle self-assigned this Sep 7, 2020
@Krinkle
Copy link
Member

Krinkle commented Sep 7, 2020

There was indeed a bug with multi-line strings. The TAP reporter handled those incorrectly, it simply printed them as-is, without considering the YAML output format that we're inside of.

However, while the fix for this was, technically valid, I think it made two mistakes:

  1. There are multiple ways to format strings in YAML. The option we choose here is in my opinion not so easy to read (due to how it encodes line breaks as \\n). It would be better I think if we use a multi-line format for multi-line strings. Reading long sequences of escaped line breaks makes debugging difficult. This is part of why I have not yet upgraded to the latest js-reporters version in QUnit.

  2. I have read the TAP spec a few times, but do not find anywhere that suggests that the data.expected/actual must contain a string for display. In fact, it shows examples that demonstrate use of other native YAML value types such as objects and arrays (not doubly-wrapped JSON strings), and it also specifically states that "the [schema] format of the data structure represented by a YAML block has not been standardized".

I think what matters most is that other command-line programs that follow the TAP spec, can parse the js-reporters TAP output, and receive the actual/expected values in their correct type. That means they should either use neatly-formatted YAML (e.g. literal numbers, multi-line strings, and native objects), or JSON (which is allowed anywhere in YAML). However even when choosing JSON, it should be as a literal, not as a doubly-wrapped string. Consumers must not need to re-parse the JSON sub-portions. It's just handled naturally by any YAML parser.

The changes I'll be making:

  • Figure out how to make strings neatly formatted over multiple lines in a way that's still valid YAML, and use that for multi-line string values.
  • Revert use of doubly-wrapped strings and doubly-wrapped JSON. Use native number literals. And for complex data structures, use native JSON as-is.

Krinkle pushed a commit that referenced this issue Sep 7, 2020
This turns too many values into quoted JSON strings, which
makes the output difficult to read.

I am reverting this for 1.x-stable to allow other changes to be
released and adopted by consumers, such as QUnit.

I will improve on the solution in a different way in master
for js-reporters 2.0.

Ref #109.
Krinkle added a commit that referenced this issue Sep 7, 2020
This is a different approach to fixing #109.

The issue was previously fixed in js-reporters 1.2.2 by
#110 but that
made multi-line strings difficult to read and left numerous
escape hatches in place. That fix has since been reverted
to keep 1.x behaving the same has before.

The new approach will be part of 2.0.

Fixes #109.
Krinkle added a commit that referenced this issue Sep 8, 2020
This is a different approach to fixing #109.

The issue was previously fixed in js-reporters 1.2.2 by
#110 but that
made multi-line strings difficult to read and left numerous
escape hatches in place. That fix has since been reverted
to keep 1.x behaving the same has before.

The new approach will be part of 2.0.

Fixes #109.
@Krinkle Krinkle closed this as completed Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants