-
Notifications
You must be signed in to change notification settings - Fork 556
Add a TestConsole #4394
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
base: series/3.x
Are you sure you want to change the base?
Add a TestConsole #4394
Conversation
33c7796
to
7319db2
Compare
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.
Is there a reason to have a separate logsRef
? It seems redundant given that we have stderr
/stdout
*/ | ||
final class TestConsole[F[_]: Parallel]( | ||
stdInSemaphore: Semaphore[F], | ||
stdInStateRef: Ref[F, TestStdInState[F]], |
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.
I'm kind of surprised at this structure and would have expected something more like a Queue
. Wouldn't it be more useful to allow users to build tests which supply input more ergonomically?
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.
I'm not sure I understand what specifically you're asking about.
If you're asking about the API, the users use TestConsole#write
and TestConsole#writeln
to supply input, they don't need to interact with the internal bits.
If you're asking about the structure of the internal bits, if this were implemented with a Queue
, it would still need something like TestStdInState
to track the partial lines, so it would only really get rid of the Semaphore
.
Since I had to work with TestStdInState
anyway, it was simpler to track the requests locally instead of delegating that to a Queue
(especially since only one state needs that functionality anyway).
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.
I've separated out the algebras that control stdIn and inspect the state of things to hopefully clarify how the user would build tests and supply input.
testkit/shared/src/main/scala/cats/effect/testkit/TestConsole.scala
Outdated
Show resolved
Hide resolved
Would it make sense to shape a test console around an ADT? We have this implementation in otel4s: https://github.com/typelevel/otel4s/blob/4b5f51093e3bb2fbb20914f901a5752a88d8e26c/sdk/common/shared/src/test/scala/org/typelevel/otel4s/sdk/test/InMemoryConsole.scala#L28 The ADT offers enough flexibility to test operations. P. S. To be fair, it doesn't support |
On the other hand, since |
Those are for capturing meta information that's useful when tests fail, like that there was For context: I initially wrote this as a local helper when writing tests for typelevel/log4cats#912, and the additional context was really helpful for debugging those tests. |
I think the main reason I went with a design that provided the full output of each stream, rather than an ADT tracking each operation, is that it would encourage writing tests using it which would be less sensitive to internal implementation changes. It feels like a smell if we encourage users to write tests that care if there's a switch under the hood from |
Implements suggestion in #4393