-
Notifications
You must be signed in to change notification settings - Fork 3.4k
ex_unit: Add :capture_io tag #14623
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: main
Are you sure you want to change the base?
ex_unit: Add :capture_io tag #14623
Conversation
This one seems solvable by picking a slightly different name, |
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 is a really neat addition 🤩 💜
Right, this doesn't play well with StringIO.flush, because then you are flushing data between tests. I think it is fine to keep this specific to stdin/stdout. It is potentially the same reason why adding a similar API to |
lib/ex_unit/lib/ex_unit.ex
Outdated
time: 0, | ||
tags: %{}, | ||
logs: "", | ||
capture_io: "", |
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.
Maybe we should call it stdout
for consistency with logs
?
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.
Good call, test.stdout
sounds good. Instead of context.capture_io
should we have context.stdout
too? I think that'd be nicer. And then same for logs, instead of context.capture_log
(or per @sabiwara context.capture_logs
) have context.logs
?
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 think the issue is that we don't want to expose %{logs: device}
because those devices are by definition shared. You will most likely read and flush data from other tests, so I think a tiny capture window works better.
In other words, we need to decide if we are happy shipping this feature considering it will only be available for stdout. The renaming to stdout
in the struct should be a separate discussion.
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.
Are they shared though? I thought each test gets its own StringIO for capturing logs.
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.
Yes, you are right.
Possible future work:
Allow
ExUnit.start(capture_io: true)
to mimiccapture_log: true
?Add api to programmatically append to the StringIO input buffer, right now it's only possible on creation:
The idea is something like this:
And so we could replace the usage of:
too.
Maybe instead of calling it
StringIO.buffer
it could be calledStringIO.input_write
andStringIO.input_puts
to mimicIO.write
andIO.puts
. Not sure if we'd need.input_binwrite
too.@tag capture_io: :standard_error
?I think we could do this as long as the test is running in
async: false
which we'd know and then we can raise otherwise. This is as opposed tocapture_io(:standard_error, ...)
where ExUnit does not know if it's sync or async.If we do this, do we want to capture both? I guess we could accept a list of devices and set it in context:
Not sure if
:stdio
is even semantically correct, my understanding is it's not stdio per se but the group leader. So yeah, it's probably a pass, and a reason to usecapture_io
in this case, but I thought I'd mention this for completeness.Make the StringIO available to
@tag :capture_log
tests in context as%{capture_log: io}
. This could be considered a breaking change since today the context is usually set as%{capture_log: true}
or%{capture_log: options}
but I can't think of a reason to read them back in a test.