-
Notifications
You must be signed in to change notification settings - Fork 47
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
Remove tracing errors from tests execution output #1142
Remove tracing errors from tests execution output #1142
Conversation
bce82b3
to
b57516c
Compare
I had not realized we were using the
I have to find a way to capture logs independently for each test. The log lines do not have any identifier to filter logs written by the execution of the test. Besides, it's not a log error we are writing directly. Maybe we should disable the |
I see two solutions: 1. Global Log CapturerWe can create a global log capturer (static) shared by all tests. The problem here is identifying logs from the test we are executing. We need to introduce an identifier. We can override the error written by tomer_http (see here). We could include the request id. Since we also have the request-id in the response, we can get it from the response header and search in logs from lines containing that request-id. 2. Write logs to filesWe can add new settings to the tracker to write logs to files. For example: [logging]
#output = 'sdt'
#output = 'file'
file_path = 'path/to/log'
format = 'plain'
#format = 'json' When we make the ephemeral test env, we can set 3. Add a logs collectorWe could create a new middleware, or tracing wrapper, or find other ways, to send logs to an external service, for example HTTP API. This could be useful in the future, but I think it's too complex now. I prefer the second option even if it can be slower because of the writing and reading to disk. At least for these tests. For other tests, maybe we should find a way to mock tracing in unit tests. |
Today, @da2ce7 and I agreed on implementing option 1 because, with option 2, the tracing configuration is also initialized once. Therefore, all tests would use the same log file. |
There are a couple of things to consider before implementing option 1:
Especially the |
Hi @da2ce7 I found this crate tracing-test that does what we want to do with option 1. However, we have to disable the tracing initialization in production with an env var when we run the tests (because we can't set a second global subscriber). I've tried to use it, but I am having a problem. I see logs written in the tests and others but I don't see some logs written from the test env. I have created a minimal example nesting levels of spawned tasks, and it's working, but some people have reported similar problems. I will continue trying more with the crate, but if I don't find the problem, we can continue with our custom solution. The crate is nice because it uses macros and remove boilerplate code from the test but the code is simple. It's similar; it has a static buffer that captures the output. Maybe we can change the configuration to use |
I've created a sample repo to reproduce the problem: https://github.com/josecelano/tracing-test-bug It does not work with threads, therefore I'm going to continue with our custom solution if I don't have the same problem. |
b57516c
to
b1e7ef5
Compare
Superseded by #1148 |
When you run tests you see some errors like this:
Those errors are written by tracing to the standard error output. They are enabled by adding these lines to the tests:
Errors are expected. It's the current API behaviour. We need to change that in the next major API version. For now, we will capture them (not showing them in the output) and assert that they were written by tracing. This is something we should do every time we write into logs, as logs are an essential feature for the application, and it should be tested, too.
This PR adds a new tracing initialization function for tests
tracing_init_with_capturer
that can be used to capture logs and assert that logs contain certain messages. For example:Subtasks
Future PRs