-
Notifications
You must be signed in to change notification settings - Fork 406
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
feat: replace custom reporter
package with slog
#1666
Conversation
d0371e6
to
7c7815a
Compare
f58ee94
to
2ea5642
Compare
2d48459
to
e752f24
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1666 +/- ##
==========================================
+ Coverage 64.55% 64.58% +0.03%
==========================================
Files 156 157 +1
Lines 15981 15840 -141
==========================================
- Hits 10316 10230 -86
+ Misses 4983 4932 -51
+ Partials 682 678 -4 ☔ View full report in Codecov by Sentry. |
5ffddc9
to
fafe77d
Compare
... and there we go: completely moved off the public There are definitely ways that this can (and should) be improved which I've already got some ideas for that I'll start working on next, but I think I'll do all of them as follow-ups since this should be the primarily (and hopefully only) breaking change that needs to be included in v2 specifically, and this PR is already very big. Currently on my list:
@another-rex let me know if you've got any thoughts on the above, or have anything to add to that list (oh and of course there's the whole "disabling of |
This comment was marked as outdated.
This comment was marked as outdated.
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.
Looking great! It looks like the tests "only" took 4 minutes on ubuntu? That's surprisingly fast all things considered.
…e `writer` field
…in the cli snapshots
Browsing patch notes as I make the migration to v2 and saw this PR.
FYI @G-Rath, a proposal (marked as likely accept) could help with this in a future Go version. |
@spencerschrock unfortunately I don't think that proposal will help - if I understand correctly, it's about the output from tests being printed which can get messy especially when tests are run in parallel, whereas our issue is that we're using a global and |
Our custom
reporter
package has served us well, but it's annoying to have to pass it through to every function especially when you're using the libraries, so this switches us over to usingslog
which uses the global default logger - the notable downside with this is as a global tests that depend on logging output cannot be run in parallel, but that shouldn't be a big deal for downstream consumers as they should be depending on the structured data returned by package functions (i.e. our packages only log information that could be useful for a human to debug stuff).This downside does impact our CLI test suite since that currently does depend on that logging output as part of feedback - for now I've just "fixed" this by removing
t.Parallel
from our CLI tests, which makes them take a lot longer but should make this landable straightaway, after which we'll be free to chop and change what we like since we'll have gotten rid of the publicreporter
package and log output is not covered by semver.I've already got a few ideas following on from this on how we might be able to speed our CLI tests back up, starting with attempting to chop the tests up into actual small packages, since Go supports running package tests in parallel regardless of their dependency on globals.
I do still need to give this a manual review, but I believe the primary thing left before this could be landed (warts or not) is to ensure when we're outputting JSON all other output goes to
stderr