Skip to content

Junit replace invalid characters#625

Merged
zwilias merged 6 commits into
rtfeldman:masterfrom
NoRedInk:junit-strip-invalid-characters
Feb 15, 2023
Merged

Junit replace invalid characters#625
zwilias merged 6 commits into
rtfeldman:masterfrom
NoRedInk:junit-strip-invalid-characters

Conversation

@zwilias
Copy link
Copy Markdown
Collaborator

@zwilias zwilias commented Feb 14, 2023

Fixes #615

Copy link
Copy Markdown
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice with a test case from #615 (making sure that the new test output of elm-program-test works).

Comment thread lib/Supervisor.js Outdated
Comment thread lib/Supervisor.js Outdated
@lydell lydell changed the title Junit strip invalid characters Junit replace invalid characters Feb 14, 2023
@zwilias
Copy link
Copy Markdown
Collaborator Author

zwilias commented Feb 14, 2023

Would be nice with a test case from #615

Yeah, I'm a little hesitant about having node-test-runner's test-suite depend on some version of elm-program-test. Given the core-issue is that it outputs control characters, and we can test that control characters are handled, I don't feel like there's a particular need for adding an external dependency to the test suite.

That said, I can confirm it resolves the issue and results in actual XML output 😄

@lydell
Copy link
Copy Markdown
Collaborator

lydell commented Feb 14, 2023

Oh, I thought it was the “▼” character that caused the problem … so elm-program-test uses \u{0008}? If it does, then the tests are all good. I didn’t mean that we should depend on elm-program-test.

Now I’m curious, why does it use control characters? (Just out of curiosity, not trying to question the need for this PR.)

@zwilias
Copy link
Copy Markdown
Collaborator Author

zwilias commented Feb 14, 2023

It uses a bunch of control characters for formatting text in the output (bold, red, green, dim, etc). This mostly means \u{001B}, though we'd want the test-runner to work with any character that's somewhat legal to have in a string 😄

I ended up with \u{0008} basically because that's the first control character that came to mind while writing a failing test for the generalised "elm-test --report junit crashes if output contains control characters" case (I don't know too many of them by heart, I have to admit.)

@lydell
Copy link
Copy Markdown
Collaborator

lydell commented Feb 14, 2023

I think \u{001B} would make for a much better test case. I immediately think of terminal escapes when I see it.

@zwilias
Copy link
Copy Markdown
Collaborator Author

zwilias commented Feb 14, 2023

Cool, I went ahead and added an extra test-case. I like having both, so as to not overfit to how it happened to be elm-program-test that surfaced the issue, while also having the clarity of something a little easier to image appearing in test output!

Comment thread lib/Supervisor.js Outdated
Comment thread tests/ci.js Outdated
zwilias and others added 2 commits February 14, 2023 16:08
Seems like we're getting closer to a final colour for the bikeshed.

Co-authored-by: Simon Lydell <simon.lydell@gmail.com>
Co-authored-by: Simon Lydell <simon.lydell@gmail.com>
Copy link
Copy Markdown
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@harrysarson do you have anything to say?

@zwilias
Copy link
Copy Markdown
Collaborator Author

zwilias commented Feb 15, 2023

I'm going to go ahead and merge this, we can always make fixups later.

@zwilias zwilias merged commit a9f85eb into rtfeldman:master Feb 15, 2023
@zwilias zwilias deleted the junit-strip-invalid-characters branch February 15, 2023 10:39
@lydell
Copy link
Copy Markdown
Collaborator

lydell commented Feb 16, 2023

Released in 0.19.1-revision12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Junit reporter breaks with new error messages of elm-program-test

2 participants