-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add JS summary handlers and some tests #20
Conversation
Erm despite the green checkmark, the tests seem to have been failing with a script error for a long time... See https://app.circleci.com/pipelines/github/loadimpact/jslib.k6.io/101/workflows/83419271-12e2-474f-a49b-aa395a8f5a62/jobs/224:
No checks whatsoever, but 0 exit code... grafana/k6#1769 will make this easier, but for now I'll add a |
Here is a test script I use to compare the new JS summary and the old Go-based summary: https://gist.github.com/na--/d68652c21a83821d0542bd52850eae28 I basically compare |
Holy moly lots of GO code! Do you want me to review the code or just verifying the output? |
Yeah... Sorry for the Go-style of code, but as I said, I decided to almost literally translate what currently exists in k6 as the first step. Once we have that and some more tests, we can refactor it and remove superfluous things (
0.0.1 is enough, I think, the code is at least usable...
Ah, sorry, I missed that, will fix it after lunch |
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.
LGTM
@legander, I fixed the formatter back to Prettier. I didn't have that VSCode module installed, so I think I just clicked |
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.
Mostly LGTM, just some nitpicks. Agree that having tests with the actual rendered summary would be good, as well as refactoring it to use "classes", etc., but we can address that later.
I'll give it a test later today.
@imiric I am currently adding a few simple tests that use |
Didn't get a chance yet to review, will hopefully be able to later today. |
@legander, thanks! I'm going to merge this in a bit, since we link to it in the v0.30.0 release notes, but if you find any major bugs in your review we can just update the |
Aight! Don't forget to add it to supported.json and run |
Didn't know I had to do that 😅 Thanks! 🙇 I probably should have read https://github.com/loadimpact/jslib.k6.io#how-to-add-a-new-js-package 🤦♂️ 🙄 |
Sorry the code is kind of 🤮 I was restricted by the need to support k6's
base
compatibility mode, and I closely followed the Go code in k6 and Go's stdlib in the manual transpilation, so that the results of the first version maximally resemble the current k6 text summary... 😞 We can change these things in the future, and we should definitely add more tests and probably refactor the code to be less of a pile of functions, but this should do for an alpha version, I think.This closes grafana/k6#1120, since it contains a very basic version of a JUnit-generating function.