-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 unit tests #3517
Add unit tests #3517
Conversation
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.
Thanks for your contribution @nilskch! I guess these bits are probably somehow covered by other tests, but I do think these shouldn't hurt, and increasing the coverage is generally positive rather than negative. Ah, it's definitely a good opportunity for you to start contributing to k6, so welcome! 👍🏻
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3517 +/- ##
==========================================
+ Coverage 72.48% 72.51% +0.02%
==========================================
Files 276 274 -2
Lines 20842 20837 -5
==========================================
+ Hits 15108 15109 +1
+ Misses 4771 4762 -9
- Partials 963 966 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@joanlopez, thank you very much! What do we do about the failing tests? I do not think they are related to my contribution. EDIT: now they all pass |
Yeah, I guess there's some flakiness. I re-ran the failed jobs twice and it end up working. |
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.
Hey @nilskch.
thanks for your contribution 🙇
Added some request changes.
cloudapi/errors_test.go
Outdated
msg1 := "some message" | ||
msg2 := "some other message" | ||
|
||
// this payload struct covers all edge cases that have not been covered before |
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.
that have not been covered before
What do you mean? It sounds like you're referring to some other tests then we should keep this new one close to them.
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.
This comment is confusing and I will remove it. This function was called somewhere else but it was not gracefully tested with all edge cases.
@codebien, I added your suggestions and squashed the commits. |
What?
I added some unit tests
Why?
Hi, thank you very much for this awesome tool! I am using it for quite some time now and LOVE it. I was looking through the code base and I like it a lot and wanted to contribute some tests.
I will eventually contribute more in the future!
Checklist
make lint
) and all checks pass.make tests
) and all tests pass. (does fail locally but my tests pass)Related PR(s)/Issue(s)