Skip to content
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

Add support for tracing-flame #49

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Apr 24, 2024

This PR is based on #48. Only the last commit belongs to this PR.

This adds support for tracing-flame. (See #47)

This PR works, but might need some work on the documentation. I am happy to follow up with it if you think the approach is good in general.

Here is how this works:

  • Enable the tracing-flame feature on test-log (this by itself doesn't to much other than adding the dependency).
  • Setting the TEST_LOG_FLAMES env-var to a path when running tests, will write traced out as {module}/{name}.folded.
  • Those can be processed with inferno as described by tracing-flames

The result of this looks something like:

image

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 28, 2024

In my opinion this functionality doesn't belong in this crate, or it will become a dumping ground for all sorts of initialization logic. I suggest you create a new crate.

@ctron
Copy link
Contributor Author

ctron commented Apr 29, 2024

Fair. What about adding some kind of "initializer" where someone could provide some fn that can hook into the process?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 29, 2024

Fair. What about adding some kind of "initializer" where someone could provide some fn that can hook into the process?

Conceptually, yes, that may be okay. That being said, I don't believe that's possible at this point. Perhaps once global attribute support is available on stable Rust that could be made to work, but until then I am not sure how you'd set global state before test-log runs, short of parametrizing the existing per-test attribute, which may end up being racy and doesn't really solve the usability issues, in my opinion.

Do you have a way to go about it?

@ctron
Copy link
Contributor Author

ctron commented Apr 29, 2024

Without having any concrete idea, I was hoping that I could get the following to work (syntax might vary):

#[test_log::test(init=init_fn)]
fn test() {}

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 29, 2024

Without having any concrete idea, I was hoping that I could get the following to work (syntax might vary):

#[test_log::test(init=init_fn)]
fn test() {}

Right, but if init_fn touches anything global this will be inherently racy: some tests set it, others may not. Depending on which runs first all may see the result, or only some. See cbcdb8c

@ctron ctron marked this pull request as draft April 29, 2024 15:18
@ctron
Copy link
Contributor Author

ctron commented Apr 29, 2024

I put this into draft state. I doesn't have a high priority for me. I'll see if I can come up with something reasonable, but don't want to make any promises.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants