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

Disable profiler in tests? #6075

Open
fjetter opened this issue Apr 6, 2022 · 5 comments · Fixed by #6490 · May be fixed by #6819
Open

Disable profiler in tests? #6075

fjetter opened this issue Apr 6, 2022 · 5 comments · Fixed by #6490 · May be fixed by #6819
Assignees
Labels
diagnostics feature Something is missing tests Unit tests and/or continuous integration

Comments

@fjetter
Copy link
Member

fjetter commented Apr 6, 2022

We constantly have a profiler running in the background, often the threads associated to it are not properly cleaned up.

In #6033 we discovered that it can also be responsible for holding on to references longer than necessary which should not be a problem in production but it is for testing.

On top of these and likely other problems, the profiler surely slows down our tests, particularly with converage enabled.

Should we not run the profiler at all for non-profiling related tests?

@fjetter fjetter added discussion Discussing a topic with no specific actions yet tests Unit tests and/or continuous integration labels Apr 6, 2022
@mrocklin
Copy link
Member

mrocklin commented Apr 6, 2022 via email

@graingert
Copy link
Member

see also #4091

@fjetter fjetter added diagnostics feature Something is missing and removed discussion Discussing a topic with no specific actions yet labels May 25, 2022
@crusaderky
Copy link
Collaborator

See also #6421

@fjetter
Copy link
Member Author

fjetter commented May 25, 2022

My proposed course of action is

  • Introduce a config flag that toggles the profiler.
  • This config flag is on by default but will be disabled in our test suite
  • All tests that rely on profiling to be enabled have to enable it specifically
  • We ensure the profiling dashboard does not crash if the toggle is off

Nice to have

  • The profiling dashboard shows a nice message indicating that profiling is off instead of trying to collect information

@graingert
Copy link
Member

we missed a few tests that start the profiler by accident see #6498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostics feature Something is missing tests Unit tests and/or continuous integration
Projects
None yet
5 participants