fix(coverage): clean ups, use normalized source code for locations #9438

DaniPopes · 2024-11-30T02:22:58Z

We pass normalized source code to solc so source locations are relative to that instead of the original source code. This is quite the footgun, I would like to see if we can remove this normalization upstream. CC @klkvr

With this PR LCOV 2.X works with no errors on the generated lcov.info file. This required adding "end line" to functions, and Line statements for all functions, see commits.

cc @grandizzy

#9437 lcov.info diff

diff --git a/lcov.info b/lcov.info
index c54fda3..3406e55 100644
--- a/lcov.info
+++ b/lcov.info
@@ -1,104 +1,131 @@
 TN:
 SF:script/Counter.s.sol
-FN:8,CounterScript.setUp
+DA:10,0
+FN:10,10,CounterScript.setUp
 FNDA:0,CounterScript.setUp
-FN:10,CounterScript.run
-FNDA:0,CounterScript.run
 DA:12,0
+FN:12,18,CounterScript.run
+FNDA:0,CounterScript.run
 DA:13,0
 DA:15,0
+DA:17,0
 FNF:2
 FNH:0
-LF:3
+LF:5
 LH:0
 BRF:0
 BRH:0
 end_of_record
 TN:
 SF:src/Counter.sol
-FN:5,Counter.setNumber
+DA:7,258
+FN:7,9,Counter.setNumber
 FNDA:258,Counter.setNumber
 DA:8,258
-FN:9,Counter.increment
-FNDA:1,Counter.increment
 DA:11,1
+FN:11,13,Counter.increment
+FNDA:1,Counter.increment
+DA:12,1
 FNF:2
 FNH:2
-LF:2
-LH:2
+LF:4
+LH:4
 BRF:0
 BRH:0
 end_of_record
 TN:
 SF:src/Cradle.sol
-FN:33,Cradle.
-FNDA:14,Cradle.
 DA:35,14
+FN:35,42,Cradle.
+FNDA:14,Cradle.
 DA:36,14
 DA:37,14
 DA:38,14
 DA:39,14
-FN:41,Cradle.disableFee
+DA:40,14
+DA:41,14
+DA:44,2
+FN:44,48,Cradle.disableFee
 FNDA:2,Cradle.disableFee
-DA:44,1
 DA:45,1
-FN:46,Cradle.setTradingEnabled
-FNDA:4,Cradle.setTradingEnabled
+DA:46,1
+DA:47,1
 DA:50,4
-FN:52,Cradle.setTransferEnabled
+FN:50,53,Cradle.setTradingEnabled
+FNDA:4,Cradle.setTradingEnabled
+DA:51,4
+DA:52,4
+DA:55,7
+FN:55,58,Cradle.setTransferEnabled
 FNDA:7,Cradle.setTransferEnabled
-DA:55,5
-FN:56,Cradle.setAmmPool
+DA:56,5
+DA:57,5
+DA:60,7
+FN:60,63,Cradle.setAmmPool
 FNDA:7,Cradle.setAmmPool
-DA:60,6
-FN:61,Cradle.setSellFee
+DA:61,6
+DA:62,6
+DA:65,5
+FN:65,70,Cradle.setSellFee
 FNDA:5,Cradle.setSellFee
-DA:65,4
-BRDA:65,0,0,1
-DA:66,1
-BRDA:66,1,0,1
-DA:67,2
-FN:67,Cradle.claimTokens
+DA:66,4
+BRDA:66,0,0,1
+DA:67,3
+BRDA:67,1,0,1
+DA:68,2
+DA:69,2
+DA:72,2
+FN:72,75,Cradle.claimTokens
 FNDA:2,Cradle.claimTokens
-DA:72,1
-FN:73,Cradle.excludeFromFees
+DA:73,1
+DA:74,1
+DA:77,2
+FN:77,80,Cradle.excludeFromFees
 FNDA:2,Cradle.excludeFromFees
-DA:77,1
-FN:78,Cradle.excludeFromLimits
+DA:78,1
+DA:79,1
+DA:82,2
+FN:82,85,Cradle.excludeFromLimits
 FNDA:2,Cradle.excludeFromLimits
-DA:82,1
-FN:83,Cradle._update
-FNDA:36,Cradle._update
+DA:83,1
+DA:84,1
 DA:87,36
+FN:87,111,Cradle._update
+FNDA:36,Cradle._update
 DA:88,36
 DA:89,36
-DA:92,23
-DA:93,15
-BRDA:93,2,0,1
-DA:95,35
-DA:96,8
-DA:99,3
-DA:100,2
-BRDA:100,3,0,2
-DA:101,33
+DA:90,36
+DA:92,36
+DA:94,23
+DA:95,1
+BRDA:95,2,0,1
+DA:97,35
+DA:99,8
+DA:101,3
 DA:102,2
-BRDA:102,4,0,2
-DA:104,2
+BRDA:102,3,0,2
+DA:104,33
+BRDA:104,4,0,2
 DA:105,2
-DA:106,33
-FN:110,Cradle.airdrop
+DA:106,2
+DA:107,2
+DA:108,2
+DA:110,33
+DA:114,3
+FN:114,119,Cradle.airdrop
 FNDA:3,Cradle.airdrop
-DA:113,2
-DA:114,1
-BRDA:114,5,0,1
 DA:115,2
-FN:116,Cradle.burn
+BRDA:115,5,0,1
+DA:116,1
+DA:117,2
+DA:121,1
+FN:121,123,Cradle.burn
 FNDA:1,Cradle.burn
-DA:117,1
+DA:122,1
 FNF:12
 FNH:12
-LF:34
-LH:34
+LF:57
+LH:57
 BRF:6
 BRH:6
 end_of_record

grandizzy · 2024-11-30T05:13:06Z

crates/evm/coverage/src/analysis.rs

+        let bytes_end = (loc.start + loc.length.unwrap_or(0)) as u32;
+        let bytes = bytes_start..bytes_end;
+
+        let start_line = self.source[..bytes.start as usize].lines().count() as u32;


there was a problem for running coverage on windows #8957 which was solved by #8958, I think will manifest again here? (wasn't able to figure out how to add a unit test)

We pass normalized source code to solc so source locations are relative to that instead of the original source code

Normalized here means in Source::new we strip '\r' from the source code.

That fix was wrong because char positions are not the same as byte positions.

grandizzy · 2024-11-30T05:31:00Z

crates/forge/src/coverage.rs

                        let name = format!("{}.{name}", item.loc.contract_name);
-                        writeln!(self.destination, "FN:{line},{name}")?;
-                        writeln!(self.destination, "FNDA:{hits},{name}")?;
+                        writeln!(self.out, "FN:{line},{line_end},{name}")?;


that's awesome, we can get rid of this from book after merged https://book.getfoundry.sh/reference/forge/forge-coverage#description

krakovia-evm · 2024-11-30T10:04:45Z

not sure if it's relevant, but i noticed comments like

/**
text
*/

breaks the coverage output (contract is basically skipped in .lcov export)

grandizzy · 2024-11-30T11:01:31Z

not sure if it's relevant, but i noticed comments like
/**
text
*/
breaks the coverage output (contract is basically skipped in .lcov export)

@krakovia-evm where exactly you put that in sources you attached? I am not able to replicate and check is solved with PR, would be great to have a unit test for the case. thanks

grandizzy

lgtm! made some more testing and lcov report shows up accurately now

krakovia-evm · 2024-11-30T11:14:43Z

not sure if it's relevant, but i noticed comments like
/**
text
*/
breaks the coverage output (contract is basically skipped in .lcov export)
@krakovia-evm where exactly you put that in sources you attached? I am not able to replicate and check is solved with PR, would be great to have a unit test for the case. thanks

Between the import & contract definition

klkvr · 2024-12-01T17:37:49Z

We pass normalized source code to solc so source locations are relative to that instead of the original source code. This is quite the footgun, I would like to see if we can remove this normalization upstream

ah, I see

the motivation for this was to ensure deterministic metadata on both windows and unix which was useful in context of #7711 but should be fine as long as project performs deployments from the same system every time

removing normalization will result in some breaking tests but there are no hard blockers for it afaict

KholdStare · 2024-12-02T20:35:06Z

@DaniPopes @grandizzy Are there any tests that actually use lcov directly? After this commit, running with lcov v1.x, it reports double the total number of functions in a file. This results function coverage always being lower than 50%. I think there should be at least some tests that pass the forge output through lcov to make sure the coverage reported by lcov remains the same.

DaniPopes · 2024-12-02T20:40:52Z

Please open an issue instead of commenting on closed PRs! I tested manually with lcov 2, I didn't try with lcov 1, sorry about that, but I also can't seem to reproduce what you said, so an issue would be great with info such as what version and how you're running lcov...

KholdStare · 2024-12-02T21:18:30Z

@DaniPopes Ok, will make a new issue with a reproduction.

I commented on the issue as I did a bisect of commits and arrived at this issue that introduced the bug.

DaniPopes · 2024-12-02T21:26:55Z

I see, thanks for bisecting

KholdStare · 2024-12-02T21:51:13Z

@DaniPopes Created the issue: #9461

DaniPopes added 2 commits November 30, 2024 02:48

feat(coverage): add function line end to LCOV, clean ups

93f9c66

fix(coverage): store normalized source code

aac108b

DaniPopes requested review from klkvr, mattsse, grandizzy, yash-atreya and zerosnacks as code owners November 30, 2024 02:22

DaniPopes added 3 commits November 30, 2024 03:31

fix(coverage): add a Line item for functions too

ebbb89d

test: update snapshots

b968251

clean

3a7efed

DaniPopes requested a review from onbjerg November 30, 2024 03:12

DaniPopes assigned DaniPopes and unassigned DaniPopes Nov 30, 2024

grandizzy reviewed Nov 30, 2024

View reviewed changes

DaniPopes mentioned this pull request Nov 30, 2024

lcov version 2.1 breaks coverage report #8281

Closed

2 tasks

grandizzy self-requested a review November 30, 2024 05:56

DaniPopes mentioned this pull request Nov 30, 2024

forge coverage branches visualization in the LCOV report #2758

Closed

grandizzy approved these changes Nov 30, 2024

View reviewed changes

grandizzy mentioned this pull request Nov 30, 2024

wrong lines in "forge coverage --report lcov" #9437

Closed

2 tasks

DaniPopes merged commit 7a23a5c into master Nov 30, 2024
21 checks passed

DaniPopes deleted the dani/coverage-cleanups branch November 30, 2024 14:47

KholdStare mentioned this pull request Dec 2, 2024

Function coverage incorrect when using lcov 1.x #9461

Closed

2 tasks

DaniPopes mentioned this pull request Dec 2, 2024

fix/feat(coverage): add --lcov-version #9462

Merged

grandizzy added T-bug Type: bug C-forge Command: forge labels Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(coverage): clean ups, use normalized source code for locations #9438

fix(coverage): clean ups, use normalized source code for locations #9438

DaniPopes commented Nov 30, 2024 •

edited

Loading

grandizzy Nov 30, 2024

DaniPopes Nov 30, 2024 •

edited

Loading

grandizzy Nov 30, 2024

krakovia-evm commented Nov 30, 2024

grandizzy commented Nov 30, 2024

grandizzy left a comment

krakovia-evm commented Nov 30, 2024

klkvr commented Dec 1, 2024

KholdStare commented Dec 2, 2024

DaniPopes commented Dec 2, 2024 •

edited

Loading

KholdStare commented Dec 2, 2024

DaniPopes commented Dec 2, 2024

KholdStare commented Dec 2, 2024

fix(coverage): clean ups, use normalized source code for locations #9438

fix(coverage): clean ups, use normalized source code for locations #9438

Conversation

DaniPopes commented Nov 30, 2024 • edited Loading

grandizzy Nov 30, 2024

Choose a reason for hiding this comment

DaniPopes Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

grandizzy Nov 30, 2024

Choose a reason for hiding this comment

krakovia-evm commented Nov 30, 2024

grandizzy commented Nov 30, 2024

grandizzy left a comment

Choose a reason for hiding this comment

krakovia-evm commented Nov 30, 2024

klkvr commented Dec 1, 2024

KholdStare commented Dec 2, 2024

DaniPopes commented Dec 2, 2024 • edited Loading

KholdStare commented Dec 2, 2024

DaniPopes commented Dec 2, 2024

KholdStare commented Dec 2, 2024

DaniPopes commented Nov 30, 2024 •

edited

Loading

DaniPopes Nov 30, 2024 •

edited

Loading

DaniPopes commented Dec 2, 2024 •

edited

Loading