Issue #19857 has been updated by mame (Yusuke Endoh). Currently only the coverage of the last call to `Kernel#eval` is kept. This seems somewhat reasonable to me because: * I don't think calling eval on the same filename has such an important use case. * If diffrent code is `eval`'d with the same filename, you get broken coverage results. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104406 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different computation is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by ioquatix (Samuel Williams). Backport changed from 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN to 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED

Currently only the coverage of the last call to Kernel#eval is kept.

This is not true. Coverage is stored in a global map of path -> coverage. Every attempt is made to use the same coverage for a given path. If you load or eval the same path multiple times, the same coverage will be reused. The bug is because `iseq_set_sequence` sets the coverage to zero for executable lines, for the lines it covers. So, it will: 1. Load the current shared coverage for a given path. 2. Based on the eval line number, update that coverage. 3. For each executable instruction, assign to the `coverage[instruction_line + eval_line_number]`. So, it won't keep the last result, it just resets some or all of the coverage, depending on how the `eval` call is invoked. In theory, the same problem can apply to `require` and `load`, but would be, I expect, a little more involved to reproduce. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104411 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different computation is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by mame (Yusuke Endoh). Status changed from Closed to Open Hey, I am the maintainer of the coverage library. Please don't merge it until I say it's okay. I'm really feeling bad smell about merging coverage results automatically. Please give me time to understand what the current situation is and to consider whether it is really OK to merge the results. So, please let me revert the commit once. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104451 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by mame (Yusuke Endoh). @ioquatix When creating a ticket, could you write up a use case? Even if you believe it is a bug, it is not necessarily so to others. You only showed an example of evaling a method definition many times. As far as that case is concerned, I don't think that it is so important and frequent that we need to care in the coverage library. Please explain how the current behavior is troubling in a real-world example. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104460 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by ioquatix (Samuel Williams). Here is an example of the ERB test issue: https://github.com/ioquatix/covered/blob/main/examples/erb/test.rb I could imagine this could show up in any code which reloads ERB templates between tests. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104477 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by ioquatix (Samuel Williams).

Thanks. In your case of ERB.new, the test function does not actually use its arguments.

I'm not sure what you mean. Depending on the argument, different coverage is generated. Can you explain why you think "the test function does not actually use its arguments"?

In this case, if we ever pass a multi-line string to test, automatic merging of the coverage results would bring trouble.

``` ERB.new(File.read(path).gsub("VALUE") { value || "default" }) ``` I understand your concern, and I have never seen such a code in the real world. Can you share the real world risk? ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104479 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by jeremyevans0 (Jeremy Evans). Merging coverage results for eval would be inconsistent with how coverage for load is handled. example code: `t.rb`: ```ruby if $a p 1 else p 2 end ``` Print coverage result: ``` # Loaded Once $ ruby -r coverage -e 'Coverage.start; $a = true; load "t.rb"; p Coverage.result' 1 {"t.rb"=>[1, 1, nil, 0, nil]} # Loaded Twice, not merged $ ruby -r coverage -e 'Coverage.start; $a = true; load "t.rb"; $a = false; load "t.rb"; p Coverage.result' 1 2 {"t.rb"=>[1, 0, nil, 1, nil]} ``` I agree with @mame that if there is no way to ensure that the same code is evaluated in each evaluation, it is wrong to merge the results. I can understand why real world code would evaluate multiple times for the same file and line: ```ruby [Array, Hash].each do |klass| klass.class_eval(RUBY, __FILE__, __LINE__+1) if self == Hash def foo; :bar end else def foo; :baz end end RUBY end ``` This is a trivial example, but at least shows why you would want to do it. In this example, it would be easy to separate the example code per class, but that is probably non-trivial and a bad idea for more complex examples. I'm not necessarily opposed to merging results, but as it is not possible to ensure result merging makes sense, it should be an option and not the default behavior (assuming that @mame is open to supporting it as an option). ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104481 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by mame (Yusuke Endoh).

By the way, did you realise, eval coverage is already merged, if it's a subset of the eval'ed code?

Yes, I think that that is bad. I think I missed it because I didn't review https://github.com/ruby/ruby/pull/6396 enough. I want to limit it even from now. I think "coverage for eval" feature was a good idea for measuring the coverage of Rails view code, but I would like to design it to the minimum necessary for that purpose. Currently, I think it is too flexible than I expected. The design I am currently considering is as follows. * If there are loaded and eval'ed codes with the same path, only the loaded code is measured * If there are multiple codes eval'd with the same path, the first (or the last) eval'ed code is measured I need to take time to understand and organize the current situation and carefully consider how it should be designed. However, I am not likely to be able to spend sufficient time to the coverage library for a while, so please wait. BTW, I'm not keen on having `Coverage.start` have a new keyword argument for this purpose. Coverage library already has many different modes. I don't want to complicate things further. (This is mostly my fault, of course.) ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-104488 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by mame (Yusuke Endoh). Status changed from Open to Rejected Sorry, but I am not keen on this proposal. Let me reject this once. When I started developing the covearge library, I decided that its target is "source files that actually exist". The measured coverage is visually observed by a human, which requires source code, and Ruby interpreter did not keep the eval'ed source code (at that time). I think that the design decision to target only source files was natural. Because the Rails view code (.erb) exists as a file, I thought that it is the target of covearge library. So I accepted #19008. Beyond that, measuring coverage on dynamically generated code conflicts with the coverage library's initial design decision. To be honest, I don't want to further complicate the coverage library. Frankly, its API is already very complicated. I no longer want to expand it beyond the initial design decision. --- One constructive note. When I first started implementing the coverage library, I had no choice but to implement it as a built-in, but now we have TracePoint, which is well-designed and general-purpose than the coverage library. Also, Prism API is being developed, and it may be possible (although I think TracePoint needs to be extended) to radically improve the coverage measurement on a column-by-column basis rather than line-by-line basis. I don't have time right now, but I would like to rebuild the library as a pure external gem in future, i.e. `coverage2` or something, with dynamically generated code, etc. in mind from the beginning. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-106238 * Author: ioquatix (Samuel Williams) * Status: Rejected * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

Issue #19857 has been updated by matthew-puku (Matthew Puku). ioquatix (Samuel Williams) wrote in #note-18:

...

I think "coverage for eval" feature was a good idea for measuring the coverage of Rails view code, but I would like to design it to the minimum necessary for that purpose. Currently, I think it is too flexible than I expected.

Actually, this bug report affects that use case too - if the same view is loaded multiple times (as demonstrated) in different tests, the coverage will not be reported correctly.

We've been running into this issue with system specs. System specs boot a child rails server, which loads the views again and clobbers coverage results from other types of spec. Our workaround is to enforce ~99.5% branch coverage, instead of 100%. It's not ideal. Having said that, our use case is probably niche. There are probably not many folks chasing 100% coverage of Rails views. ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857#change-114231 * Author: ioquatix (Samuel Williams) * Status: Rejected * Assignee: ioquatix (Samuel Williams) ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different coverage is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/