From: eregontp@... Date: 2016-04-02T18:17:40+00:00 Subject: [ruby-core:74789] [Ruby trunk Bug#12220] Why does Coverage keep previously-loaded files as empty arrays? Issue #12220 has been updated by Benoit Daloze. Yusuke Endoh wrote: Thank you for your reply. > Benoit Daloze wrote: > > I consider it a bug in that it seems to be unintentional and it is just a side-effect of the implementation to disable coverage when reset. > > It was actually intentional. Please read the test case of #4796. Yes, I already read it but it seems hard to understand the wanted behavior. The particular test case shows nothing of the wanted behavior unfortunately. I also don't get it, if one wants to use multiple coverage results, then one can just get the keys of the previous result(s) to find the "filenames with empty arrays". > It explicitly requires the second call of `Coverage.result` return non-empty hash after restarted. I cannot remember the rationale precisely, but I think keeping the filename was critical, at least, for his use case. So, returning an empty Hash breaks the compatibility. Right, there is a compatibility concern, and I would like to check simplecov about it. But I really do not see how these empty arrays are useful. > I think of some possibilities to make it "intuitive". One is your patch. I think it is the same behavior as my first implementation. But I'm afraid if it breaks compatibility. > Another is clearing all counters (to 0) instead of clearing the array itself, as Tsuyoshi Sawada and #9572 proposed. I don't really like this because I expect the same code to be excluded again after restarted. But if I have to do something, this seems the most reasonable choice. Arrays with zeroes seem like they should keep incrementing when those lines are executed. It's also expensive to know whether it's all zeros and might be difficult to differentiate with never-executed code. It seems #9572 would be much better handled with #peek_result. It seems the OP does not want restart as I understand it (stop existing coverage, start with a fresh state, as if the second Coverage.start was the first time coverage was started). > Anyway, I feel defensive to change the API even if it is a little awkward, unless there is actual use case. It also means that other implementations should follow this awkward side of the API if they want to present the same behavior. As we know, both in MRI and ruby/spec, it does prevent to make accurate tests without additional efforts (filtering out the empty arrays). And lack of these tests made such a strange feature mostly hidden for everyone, lost the true purpose of the change, and do not document the expected behavior clearly. The current state is also inconsistent with the limitation of coverage, which seems the major cause of confusion and therefore should be as simple as possible: only files loaded after a Coverage.start are tracked. With the empty arrays we have these "pseudo previously-tracked files" lying around. In the case the same file is loaded and coverage-tracked twice, the previous empty array also "magically" disappears and coverage is tracked again for that file. In the end I think this boils down to the meaning of Coverage.result. If we have "restartable" or "reset-able" coverage with Coverage.result, then the only logical result is to have a fully-reset state (at the user level). If not, as currently, it means there is leaking global state and no way for two usages of Coverage (on different files) to not impact each other. I could see two libraries using Coverage and calling #start/#result to get information on what is executed while they are being required. Why should the first library influence the second one? If we do not have that, then what do we have? According to RDoc it would just "disable coverage measurement", but what is that? Incomplete measurements? #peek_result is much better for those "incremental" use-cases. ---------------------------------------- Bug #12220: Why does Coverage keep previously-loaded files as empty arrays? https://bugs.ruby-lang.org/issues/12220#change-57913 * Author: Benoit Daloze * Status: Open * Priority: Normal * Assignee: Yusuke Endoh * ruby -v: ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-linux] * Backport: 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN ---------------------------------------- For instance, ruby -e 'require "coverage"; Coverage.start; require "set"; Coverage.result; Coverage.start; require "tmpdir"; p Coverage.result' {".../lib/ruby/2.2.0/set.rb"=>[], ".../lib/ruby/2.2.0/tmpdir.rb"=>[nil, nil, nil, nil, nil, nil, 1, 1, ...]} So Coverage.result stops coverage but also does some cleaning up. I think the most intuitive would be that the coverage Hash would be reset to be empty, but instead it's filled with empty arrays. As an example, this makes it fairly awkward to test and it enforces irreversible global state (https://github.com/ruby/spec/pull/219). I also do not see how this would be useful for coverage libraries. Is there a reason for this behavior? If not, would it be OK to clear the Hash when calling #result? ---Files-------------------------------- 0001-ext-coverage-coverage.c-Fully-reset-coverage-to-not-.patch (5.14 KB) -- https://bugs.ruby-lang.org/ Unsubscribe: