-
-
Notifications
You must be signed in to change notification settings - Fork 391
Add configurable logging to withArg & withNullableArg #1441
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
Conversation
|
Thanks for the investigation! What if instead of just passing a flag to turn on/off the That would also solve at least one of the future improvements you identified. |
|
Hi @Raibaz . Thats an interesting idea. How do you think we should handle the default there to maintain the current functionality? Nullable with default null, and if its null keep current behaviour, otherwise use the provided logger? Should we use a WARN or maybe even INFO level? Or it could be something like this, that way when we dont want any logging, we can even completely skip the logger overhead, which for a lot of larger strings (like in this case) can actually have a noticable impact and we can leave it up to the user to use whatever log level they deem appropriate then we could call the function e.g. like this: edit: actually that wouldnt work, since the receiver param would be the assertionError 🤔 would work with a |
|
Uhm, why wouldn't it work? We could implement the And change the I quite like it, as it offer good customizability; I actually like it way more than just passing a logger. |
|
i meant that a very short and clean reference pass wouldnt work, ie |
|
ok, so i think this signature works quite well, some example usages one more thing Im considering: since right now the default function body is defined the same in quite a few places, could it be moved to possibly some static value? Where could this go, is there some file containing different util functions etc? Or maybe some config class that could be a good place to put it? |
|
Can you please run |
|
ok, should be done. Thanks for letting me know which exactly to run :) |
|
Thanks! |
| * | ||
| * @param logAllAssertionErrors if true, all assertion errors inside the captureBlock will be logged. | ||
| * This can cause multiple errors to be logged if the matcher is evaluated | ||
| * multiple times. |
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 doesn't match the actual parameter of the method?
| * @return the captured argument | ||
| */ | ||
| inline fun <reified T : Any> withArg( | ||
| noinline assertionErrorLogger: (AssertionError) -> Unit = { e -> e.printStackTrace() }, |
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 breaks compilation for uses of withArg with a variable (instead of a direct given block)...
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.
@hho can you provide an example please?
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.
Sure. Here's what broke for me (only anonymized):
My capture block is extracted into the someDetailValidator variable. This worked fine in 1.14.6, but breaks in 1.14.7, because now I need to either pass an assertionErrorLogger or spell out the captureBlock parameter name.
I think the problem might've be avoidable, if the new signatures (with the new assertionErrorLogger parameter) would've just been added as separate methods, without a default for the logger?
|
Maybe I don't completely understand what's going on here, but I believe such a breaking API change shouldn't be hidden in a patch release? |
Problem decription:
After upgrading to a newer version I noticed a LOT of spamming in the console logs, enough to significantly slow down test execution and even crash the IDE. After some research I narrowed it down to this PR: #1395
The issue I encountered is caused by a combination of these factors:
withArgandwithNullableArgdumping the error stack trace on any failed match, even if the test itself passesProposed solution:
Add an argument flag to the
withArgandwithNullableArgfunctions to toggle the stacktrace dump behaviourReproduction:
build.gradle.kts
Foo.kt
FooSpec.kt
Here the first testcase spams the console with messages like this:

Possible future improvements:
Would be nice if the behaviour could be configured project-wise via mockk config (would love some pointers on how to achieve this).
Would be nice if the stacktrace dump actually used a logger instead of directly dumping to console.