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

Memory leak fix: release console output reference after printed to stdout. #8233

Merged
merged 6 commits into from
Mar 29, 2019

Conversation

scotthovestadt
Copy link
Contributor

Summary

The reference to the console output is printed to stdout when the test completes and not used afterwards. Unfortunately, it's currently preserved, never being released. It's not used after that point and can be garbage collected.

I also standardized the "no buffer" output to undefined, when it previously could have been an empty array or null.

Test plan

  • All tests pass.
  • Console output still works as expected.
  • Memory does not leak.
  • JSON output is unchanged.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)

),
);
result.console = undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the reporter should be responsible for mutating the testresult object. The memory consumption would be reintroduced with custom reporters. Can we move this to the runner or jest-core instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feedback, moved to jest-core. To be honest, I just blindly followed the pattern of how coverage was previously released without thinking about it too carefully. Thanks for your review.

@codecov-io
Copy link

Codecov Report

Merging #8233 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8233   +/-   ##
=======================================
  Coverage   62.33%   62.33%           
=======================================
  Files         265      265           
  Lines       10553    10553           
  Branches     2565     2567    +2     
=======================================
  Hits         6578     6578           
  Misses       3387     3387           
  Partials      588      588
Impacted Files Coverage Δ
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts 0% <ø> (ø) ⬆️
packages/jest-test-result/src/helpers.ts 0% <ø> (ø) ⬆️
packages/jest-console/src/CustomConsole.ts 68.75% <0%> (ø) ⬆️
packages/jest-reporters/src/default_reporter.ts 82.6% <0%> (-0.25%) ⬇️
packages/jest-console/src/BufferedConsole.ts 70% <100%> (ø) ⬆️
packages/jest-core/src/ReporterDispatcher.ts 82.35% <100%> (+1.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e08be02...eb1d72c. Read the comment docs.

@alexey-temnikov
Copy link

This fix broke compatibility with ability to print console logs in the reports (like jest-html-reporter).
As console object is cleared up onTestResult it is not available any longer for other reports during onRunComplete in other reports so logs are eventually going missing.

The feature to incorporate certain logs from the test execution into the report is quite useful.

Reference issue is #Hargne/jest-html-reporter#61

Rather than completely disabling it, it would be great to see a parameter / hook, so developers have ability to define, if they want to keep logs, throw them right away, or process in a certain way (e.g. persist in a report file, or shorten, to avoid huge memory consumption in case tests generate huge amount of logs). Or at least document that capability (as it can be currently supported by using custom reporter).

@SimenB
Copy link
Member

SimenB commented May 27, 2019

We can just move the cleanup to a later part of the lifecycle if needed - I think that's cleaner than more config. Mind opening up a separate issue with a quick reproduction?

@scotthovestadt
Copy link
Contributor Author

It definitely should not be available in onRunComplete, since by definition that means it'll be held onto for the entire run time. This simply doesn't scale past a certain point.

The reporter should be able to get at the console logs as each test completes individually and store them for later access if needed.

If this doesn't work for some reason file an issue and I'm willing to fix that.

@alexey-temnikov
Copy link

@SimenB, @scotthovestadt
Thank you for your blazing fast reply :)

It make sense, reporter shall hook into onTestResult and efficiently (minimizing memory footprint) store logs. Perhaps we will see that support in future.

For a quick and dirty WA to make current reporters compatible with a newer jest version (again reference to jest-html-reporter) I'm just "restoring" the value for console variable in a custom "fake" reporter, hooking into onTestResult.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants