Bug #12220: Why does Coverage keep previously-loaded files as empty arrays?

• Status: Closed
• Assignee: Eregon (Benoit Daloze)
• Target version: -
• ruby -v: ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-linux]
• Backport: 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN

[Eregon (Benoit Daloze)]

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?

[mame (Yusuke Endoh)]

Coverage is a very special module. Please use it just as rdoc says:

* = Usage
*
* 1. require "coverage"
* 2. do Coverage.start
* 3. require or load Ruby source file
* 4. Coverage.result will return a hash that contains filename as key and
*    coverage array as value. A coverage array gives, for each line, the
*    number of line execution by the interpreter. A +nil+ value means
*    coverage is disabled for this line (lines like +else+ and +end+).

I don't care the behavior of restarting coverage because I have no actual use case. Do you have any? I don't think that ruby/spec is.

This module is considered professional-use. With regard to this module, intuition is not important to me, unless there is actual use case.

From the beginning, I had given up making this module "intuitive". I think the most intuitive usage is (1) requiring all modules, next (2) call Coverage.start, (3) do the main task, and finally (4) call Coverage.result and save the result. But it is impossible because of an implementation restriction (the compiler does not insert trace instructions by default because of its overhead).

If not, would it be OK to clear the Hash when calling #result?

As I recall correctly, the first implementation did so, but the behavior changed by an ticket #4796. I don't recall why it is needed, but the OP seemed to think it useful.

--
Yusuke Endoh [e-mail]

[sawa (Tsuyoshi Sawada)]

Yusuke Endoh wrote:

As I recall correctly, the first implementation did so, but the behavior changed by an ticket #4796. I don't recall why it is needed, but the OP seemed to think it useful.

I might be wrong, but my interpretation is that Benoit Daloze wants the key-value pairs to be removed when the value is an empty array, but Yusuke Endoh is taking it as a request to retain the previous counts. I think there is a miscommunication here.

I personally agree with Benoit Daloze that the current behaviour can be considered a bug. It retains a key with an empty array, which is supposed to be gone if whatever loaded prior to the first Coverage.result should be irrelevant to the coverage count after it.

And the fix should be trivial. There should be no implementation difficulty.

[mame (Yusuke Endoh)]

Tsuyoshi Sawada wrote:

Yusuke Endoh wrote:

As I recall correctly, the first implementation did so, but the behavior changed by an ticket #4796. I don't recall why it is needed, but the OP seemed to think it useful.

I might be wrong, but my interpretation is that Benoit Daloze wants the key-value pairs to be removed when the value is an empty array, but Yusuke Endoh is taking it as a request to retain the previous counts. I think there is a miscommunication here.

I think I correctly understood eregon's request. As far as I recall, the first implementation actually did completely remove the key-value pairs when Coverage.result is called, as eregon requested. Did you read #4796?

--
Yusuke Endoh [e-mail]

[sawa (Tsuyoshi Sawada)]

Yusuke Endoh wrote:

the first implementation actually did completely remove the key-value pairs when Coverage.result is called, as eregon requested.

I can't find what eregon's request is.

Did you read #4796?

I may have not understood it. But, isn't returning an empty array meaningless (unless the file is empty)? If set.rb is to be returned in the result but has not been called, it should return an array filled with nil or 0:

".../lib/ruby/2.2.0/set.rb"=>[nil or 0, nil or 0, ..., nil or 0],      # n `nil` or `0`s, where `set.rb` is n-lines long

instead of an empty array:

".../lib/ruby/2.2.0/set.rb"=>[],

[mame (Yusuke Endoh)]

Tsuyoshi Sawada wrote:

Yusuke Endoh wrote:

the first implementation actually did completely remove the key-value pairs when Coverage.result is called, as eregon requested.

I can't find what eregon's request is.

Heh, you said "my interpretation is that Benoit Daloze wants the key-value pairs to be removed when the value is an empty array". My interpretation is the same.

Did you read #4796?

I may have not understood it. But, isn't returning an empty array meaningless (unless the file is empty)? If set.rb is to be returned in the result but has not been called, it should return an array filled with nil or 0:

Your request is a duplicate of #9572. In that ticket, Sam Rawlins said "I'm not really in favor of knowingly reporting inaccurate data on restart". I agree with him. In addition, now there is a better API, Coverage.peek_result, to calculate a coverage difference. Currently I have no motivation to change the behavior of Coverage.result.

--
Yusuke Endoh [e-mail]

[Eregon (Benoit Daloze)]

Yusuke Endoh wrote:

Thank you for your answer!

I don't care the behavior of restarting coverage because I have no actual use case. Do you have any? I don't think that ruby/spec is.

Indeed, although tests usually improve the design of an API.

This module is considered professional-use. With regard to this module, intuition is not important to me, unless there is actual use case.

I think the current behavior is more confusing than helping, so the use case would be "avoid unexpected behavior :)".

From the beginning, I had given up making this module "intuitive". I think the most intuitive usage is (1) requiring all modules, next (2) call Coverage.start, (3) do the main task, and finally (4) call Coverage.result and save the result. But it is impossible because of an implementation restriction (the compiler does not insert trace instructions by default because of its overhead).

This is fine as a limitation of the module, no objection to that.

If not, would it be OK to clear the Hash when calling #result?

As I recall correctly, the first implementation did so, but the behavior changed by an ticket #4796. I don't recall why it is needed, but the OP seemed to think it useful.

It seems to be an unintended side-effect of your patch in #4796.
By "restartable", I think most people would expect an initial state again (no coverage info, so an empty Hash or Qundef internally).

Then the pair Coverage.start/Coverage.result truly allow to restart/reset and Coverage.peek_result allows to read the current state.

That seems like a good API, and as far as I can see the only change would be to fully reset coverage data on Coverage.result.
I'll attach a patch.

[Eregon (Benoit Daloze)]

After looking into this, I discovered a bug!
And I also found the reason why it's returning empty arrays: it is leaking implementation details.
It is however easy to fix but let's start with fixing the bug first.

[sawa (Tsuyoshi Sawada)]

So, was it or was it not a bug that it returns empty arrays?

[Eregon (Benoit Daloze)]

• File 0001-ext-coverage-coverage.c-Fully-reset-coverage-to-not-.patch 0001-ext-coverage-coverage.c-Fully-reset-coverage-to-not-.patch added

Tsuyoshi Sawada wrote:

So, was it or was it not a bug that it returns empty arrays?

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.

I attach a patch that shows what I would like to change:
Not returning empty arrays to the user.
Update to the tests show this API feels more natural if Coverage#result resets the state.
I do not see any useful purpose for these arrays for the user, so I think this change is fairly compatible and in fact makes the returned result less surprising.
It also removes additional global state, which is always a win.

Internally, empty arrays are kept as markers to not track coverage anymore, but they are never exposed to the user.

@mame (Yusuke Endoh): Do you agree with this patch?

[mame (Yusuke Endoh)]

Benoit Daloze wrote:

Tsuyoshi Sawada wrote:

So, was it or was it not a bug that it returns empty arrays?

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.

        Coverage.start
        require tmp + '/test.rb'
        Coverage.result
        Coverage.start
        coverage_test_method
        assert_equal 1, Coverage.result.size # <=== HERE

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.

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.

Anyway, I feel defensive to change the API even if it is a little awkward, unless there is actual use case.

Thank you,

[sawa (Tsuyoshi Sawada)]

Yusuke Endoh wrote:

It was actually intentional. Please read the test case of #4796.

        Coverage.start
        require tmp + '/test.rb'
        Coverage.result
        Coverage.start
        coverage_test_method
        assert_equal 1, Coverage.result.size # <=== HERE

It explicitly requires the second call of Coverage.result return non-empty hash after restarted.

That is because the method coverage_test_method actually executes some lines in "/tmp/test.rb", and so the corresponding array should not be empty in such case. I don't see how this is relevant to the issue of whether or not to delete key-value pairs from the hash when the array value is empty.

[mame (Yusuke Endoh)]

I'm unsure what behavior you expect. Do you mean that it should pass the following two tests?

Coverage.start
require tmp + '/test.rb'
Coverage.result
Coverage.start
assert_equal {}, Coverage.result

Coverage.start
require tmp + '/test.rb'
Coverage.result
Coverage.start
coverage_test_method
assert_equal 1, Coverage.result.size # what should Coverage.result return precisely?

[sawa (Tsuyoshi Sawada)]

Yusuke Endoh wrote:

I'm unsure what behavior you expect. Do you mean that it should pass the following two tests?

I expect the second Coverage.result to return:

{"/tmp/test.rb" => [0, 1, nil]}

In other words, Coverage.result.size should return 1.

[mame (Yusuke Endoh)]

Then, you want Coverage.result to remove all arrays from the Hash once, keep them in back, and automatically restore an array when the corresponding code is executed? This is the third idea to make it "intuitive" that I thought of but didn't mention. It is so complex and I don't see any use case.

[sawa (Tsuyoshi Sawada)]

Yusuke Endoh wrote:

Then, you want Coverage.result to remove all arrays from the Hash once, keep them in back, and automatically restore an array when the corresponding code is executed?

I am not sure what you mean by "keep them in back, and automatically restore". I expect Coverage.result to remove all arrays, period. (Or to be more direct, clear/throw out the entire hash.) (The second) Coverage.start should just start with a fresh new hash.

Under any interpretation, the first Coverage.result should return:

{"/tmp/test.rb" => [1, 0, nil]}

and if the hash/array were "kept in the back", the second Coverage.start would have no effect, and the second Coverage.result would return:

{"/tmp/test.rb" => [1, 1, nil]}

In reality, prior to the change in response to #4796, it returned:

{}

My interpretation of #4796 is that, it is asking that, instead of {}, we should get:

{"/tmp/test.rb" => [0, 1, nil]}

Yusuke Endoh wrote:

This is the third idea to make it "intuitive" that I thought of but didn't mention. It is so complex and I don't see any use case.

My guess regarding where the difference lies among us is that I have in mind that load or require method is not necessary for a key-value pair to be introduced into the hash; whenever a line in an outer file is executed, that creates a new key-value pair in the hash if not any.

[Eregon (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.

[xshay (Xavier Shay)]

Hello, I filed #4796. It was a long time ago, and I don't remember what I was trying to do. Likely something around a "coverage per test" feature.

Right, there is a compatibility concern, and I would like to check simplecov about it.
it does prevent to make accurate tests without additional efforts (filtering out the empty arrays).

I don't feel making it "easier to write specs" is sufficient reason to change this, but if simplecov test suite passes with it (pretty sure it doesn't rely on restartable coverage) then I don't have a strong objection.

[mame (Yusuke Endoh)]

• Assignee changed from mame (Yusuke Endoh) to Eregon (Benoit Daloze)

Xavier, thank you for your opinion. I also don't think that it is a good idea to change because of a spec, but I don't have a strong objection if the OP of #4796 is satisfied. So I agree with eregon's patch. Eregon, could you commit it?

I think this is not a bug but a feature, but I'd like to leave the decision to a branch maintainer.

--
Yusuke Endoh [e-mail]

[Eregon (Benoit Daloze)]

Thank you for your reply.

Xavier Shay wrote:

Hello, I filed #4796. It was a long time ago, and I don't remember what I was trying to do. Likely something around a "coverage per test" feature.

Right, there is a compatibility concern, and I would like to check simplecov about it.
it does prevent to make accurate tests without additional efforts (filtering out the empty arrays).

I don't feel making it "easier to write specs" is sufficient reason to change this,

Please read my argument fully, following that is:
"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."

So this means, that up to this day, nobody understands this feature wells, and good tests would uncover that and most likely cause us to discuss it.
So that's what I am trying to do, get Coverage tested properly so that other implementations can be compatible. Is that not an important concern?

but if simplecov test suite passes with it (pretty sure it doesn't rely on restartable coverage) then I don't have a strong objection.

SimpleCov tests pass with this modification, and indeed they do not use restartable coverage (spec/ does not use Coverage.result, and Cucumber features spawn different processes).

If some code used Coverage before SimpleCov starts to use it, then SimpleCov would get empty arrays from Coverage.result.
This could have an impact for instance on add_not_loaded_files, which could report files as empty instead of not covered.

The merging results feature seems like it would be unaffected due to its implementation:
new_resultset[filename] = (self[filename] || []).extend(SimpleCov::ArrayMergeHelper).merge_resultset(hash[filename] || [])
Another implementation could skip the merging if only one side exists, and empty arrays could then cause additional processing.

I think this is a good enough example that the current behavior of restartable Coverage is harmful and unhelpful.
As it is rarely used in practice it was not yet reported but nevertheless it is worthy to fix,
if only to not leak this implementation detail bug to other implementations and future libraries using Coverage.

[Eregon (Benoit Daloze)]

Yusuke Endoh wrote:

Xavier, thank you for your opinion. I also don't think that it is a good idea to change because of a spec, but I don't have a strong objection if the OP of #4796 is satisfied. So I agree with eregon's patch. Eregon, could you commit it?

Will do, thank you!

I think this is not a bug but a feature, but I'd like to leave the decision to a branch maintainer.

Of course, this part is unclear for me too for older versions.
It would be nice if it behaves consistently on updates of existing releases though.

[Eregon (Benoit Daloze)]

• Status changed from Open to Closed

Applied in changeset r54504.

---

• ext/coverage/coverage.c: Fully reset coverage to not persist global state.
  It was returning old file coverages as empty arrays to the user.
  [ruby-core:74596] [Bug #12220]
• ext/coverage/coverage.c (rb_coverages): remove unused static state.
• thread.c: Moved and renamed coverage_clear_result_i to reset_coverage_i.
• test/coverage/test_coverage.rb: improve precision of tests.