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

Don't discard all information about passing tests immidately #25483

Closed
oxinabox opened this issue Jan 10, 2018 · 5 comments · Fixed by #36879
Closed

Don't discard all information about passing tests immidately #25483

oxinabox opened this issue Jan 10, 2018 · 5 comments · Fixed by #36879
Labels
testsystem The unit testing framework and Test stdlib

Comments

@oxinabox
Copy link
Contributor

oxinabox commented Jan 10, 2018

Right now the testing infrastructure itself discards all information for passing tests.
The Pass result type has fields for all the information, and a show method that would display it, but they are all blanked out with nothing etc as their values.

value ? Pass(:test, nothing, nothing, value) :

Determining whether or not to do this should not be done there.
It should be done in the record method of the testset.
That way it can be configured.

record(::DefaultTestSet, ::Pass) already discards the pass result entirely anyway.

I don't even think doing it later will cost memory, all that information is being captured anyway so that it can be reported for Failure/

@ararslan
Copy link
Member

I believe the fact that information about passing tests is discarded is actually deliberate. I'm having a difficult time finding a discussion here on GitHub, but I recall that there was an issue with the amount of memory that large test sets were racking up since they were retaining too much information. I thought the resolution to that was to discard information about passes, but I could be misremembering this entirely.

@ararslan ararslan added the testsystem The unit testing framework and Test stdlib label Jan 10, 2018
@oxinabox
Copy link
Contributor Author

oxinabox commented Jan 10, 2018

I think that is covered by discarding the whole Pass result

# For a broken result, simply store the result
record(ts::DefaultTestSet, t::Broken) = (push!(ts.results, t); t)
# For a passed result, do not store the result since it uses a lot of memory
record(ts::DefaultTestSet, t::Pass) = (ts.n_passed += 1; t)

It doesn't need to be empty when discarded -- it is being constructed (with nothings) then discarded.
It might as well be constructed filled,
and then TestSets can override the behavior of what to do with it. Default can still discard it, and other TestSets can instead add it to results.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jan 10, 2018

I believe the fact that information about passing tests is discarded is actually deliberate. I'm having a difficult time finding a discussion here on GitHub, but I recall that there was an issue with the amount of memory that large test sets were racking up since they were retaining too much information. I thought the resolution to that was to discard information about passes, but I could be misremembering this entirely.

#20150

@oxinabox oxinabox changed the title Don't discard all information about passing tests Don't discard all information about passing tests immidately Jan 11, 2018
@oxinabox
Copy link
Contributor Author

So the PR that introduced this discarding of the fields of Pass was
#18738
(One of many things it did)

@kshyatt am I right in saying this was an earlier attempt at decreasing memory use,
and with the changes from #20150 that it can be undone, so we just discard pass wholely.

mmiller-max added a commit to mmiller-max/julia that referenced this issue Jul 23, 2020
@mmiller-max
Copy link
Contributor

Just bumping this, as retaining the pass information will help greatly in the writing of test reports which we are doing in TestReports.jl.

I've got the changes ready for a PR here, but first it'd be good to have confirmation here that this will not have any negative performance impacts (because of the changes made in #20150).

mmiller-max added a commit to mmiller-max/julia that referenced this issue Jul 24, 2020
mmiller-max added a commit to mmiller-max/julia that referenced this issue Aug 2, 2020
mmiller-max added a commit to mmiller-max/julia that referenced this issue Aug 2, 2020
mmiller-max added a commit to mmiller-max/julia that referenced this issue Aug 3, 2020
Remove printing of detailed Pass information, not used before this change
mmiller-max added a commit to mmiller-max/julia that referenced this issue Aug 5, 2020
Remove printing of detailed Pass information, not used before this change
mmiller-max added a commit to mmiller-max/julia that referenced this issue Aug 23, 2020
Remove printing of detailed Pass information, not used before this change
mmiller-max added a commit to mmiller-max/julia that referenced this issue Apr 27, 2021
mmiller-max added a commit to mmiller-max/julia that referenced this issue Apr 27, 2021
mmiller-max added a commit to mmiller-max/julia that referenced this issue Apr 28, 2021
vtjnash pushed a commit that referenced this issue May 29, 2021
shirodkara pushed a commit to shirodkara/julia that referenced this issue Jun 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this issue Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants