Implement initial LCOV reporter (no function names support) #11883

exoego · 2024-06-15T00:30:04Z

What does this PR do?

This adds new config to bunfig which allows users to use lcov coverage reporter and also disable console reporter.

Documentation or TypeScript types (it's okay to leave the rest blank in this case)
Code changes

AFAIK, function names in LCOV is optional, and tools like CodeCov accept LCOV file without function names.
So I think LCOV without function names is still valuable to users.

How did you verify your code works?

I run the locally-built bun-debug over https://github.com/honojs/hono and obtained LCOV file like below:

...
TN:
SF:src/adapter/bun/index.ts
FNF:1
FNH:1
DA:6,45
DA:7,51
DA:8,49
DA:9,40
LF:10
LH:4
end_of_record
...

Then I uploaded the generated LCOV to CodeCov and confirmed files are succesfully processed by CodeCov:

Sorry, I couldn't figure out how to write test for this.
Happy to add test if guidance provided 🙇

I included a test for the new code, or an existing test covers it
I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)
I updated Aliases in module_loader.zig to include the new module

github-actions · 2024-06-17T22:23:37Z

❌ @Jarred-Sumner, your commit has failing tests :(

💻 1 failing tests Darwin x64 baseline

test/regression/issue/09041.test.ts 1 failing

🐧💪 1 failing tests Linux AARCH64

test/js/deno/crypto/webcrypto.test.ts 1 failing

🪟💻 1 failing tests Windows x64 baseline

test/cli/install/registry/bun-install-registry.test.ts 1 failing

🪟💻 2 failing tests Windows x64

test/integration/next-pages/test/dev-server.test.ts 1 failing
test/js/bun/io/bun-write.test.js 1 failing

View logs

exoego · 2024-06-21T01:20:12Z

I am happy to finish this PR if I get advised what is missing.

paperclover

overall this looks good. some notes attached.

in addition

kind of sad that lines with comments are marked as not hit. though i dont think that is an issue in your implementation, but the coverage stuff overall.
should we get --coverage-reporter= command line arguments?
for tests, I think it would be reasonable to snapshot test the samples. look for usages of tempDirWithFiles to setup directory trees to test.

src/cli/test_command.zig

paperclover · 2024-06-21T01:50:08Z

src/cli/test_command.zig

+
+        while (iter.next()) |entry| {
+            const value: bun.sourcemap.ByteRangeMapping = entry.*;
+            byte_ranges.appendAssumeCapacity(value);


cant this just be written as appendAssumeCapacity(value.*)

No, it doesn't compile on my end:

src/cli/test_command.zig:292:51: error: cannot dereference non-pointer type 'src.sourcemap.CodeCoverage.ByteRangeMapping' byte_ranges.appendAssumeCapacity(value.*); ~~~~~^~ src/sourcemap/CodeCoverage.zig:307:30: note: struct declared here pub const ByteRangeMapping = struct { ^~~~~~ referenced by: exec: src/cli/test_command.zig:920:37

oh wait sorry, i meant entry.*

the point being these two lines can be one line (i dont think the type annotation is of much value)

Oh, it worked 54d12d3
(Sorry, I am a super newbie to Ziglang)

src/cli/test_command.zig

src/bunfig.zig

exoego · 2024-06-21T06:32:16Z

@paperdave Thanks for your review. I think all comments are addressed

paperclover · 2024-06-21T06:52:09Z

@exoego see the two bullet points in the main 'requested changes' comment. to repeat:

this at least needs tests to verify it works on all platforms, and to make sure it doesnt just stop working one day.
consider (but not mandatory) adding a CLI flag like --coverage-reporter

but otherwise we're good

exoego · 2024-06-21T08:02:33Z

Added CLI parameters in 68d99da

Added snapshot test 75476db
But bun test --update-snapshots does not even create coverage/ directory...
I miss some steps to test bun changes?

EDIT:
I was succesfully able to run npm run test and commit snapshot .

paperclover

im going to add a commit that renames --coverage-reports-dir to just --coverage-dir but yeah this is good.

it could use more tests, but i want to get this in for todays release.

paperclover

wait, nevermind, coverage.test.ts is failing

exoego · 2024-06-22T04:15:22Z

Hmm, do you have an advice to fix coverage.test.ts?

…make it one pass, use more Windows-friendly APIs for creating the directory

Jarred-Sumner · 2024-06-22T07:45:39Z

src/sourcemap/CodeCoverage.zig

-        while (iter.next()) |line| {
-            // DA: line number, hit count
-            // FIXME: Remove the magic number that is used to convert the hit count to the correct value
-            try writer.print("DA:{d},{d}\n", .{ line + 1, report.lines_hits.ptr[line] - 2863311530 });


@exoego the reason this magic number made it seemingly work: undefined memory. In safety-checked builds of Zig (debug, ReleaseSafe), 0xa is the default-initialized value for memory.

This magic number was turning 0xa into 0. In a release build, this is undefined behavior and will produce a result which will either crash the program, infinite loop, or something unexpected. This is why it did not work in CI.

Jarred-Sumner · 2024-06-22T07:47:20Z

src/sourcemap/CodeCoverage.zig

+            line_hits = try LinesHits.initCapacity(allocator, line_count);
+            line_hits.len = line_count;
+            const line_hits_slice = line_hits.slice();
+            @memset(line_hits_slice, 0);


@exoego note how we now zero-initialize the line_hits array to ensure that we can access values which have never been written to without worrying about undefined behavior.

Jarred-Sumner · 2024-06-22T07:50:17Z

src/sourcemap/CodeCoverage.zig

@@ -415,7 +477,7 @@ pub const ByteRangeMapping = struct {
                    executable_lines.set(line);
                    if (has_executed) {
                        lines_which_have_executed.set(line);
-                        lines_hits.ptr[line] += 1;
+                        line_hits_slice[line] += 1;


@exoego writing to a many-item pointer ([*]MyType) directly is very much a footgun. You want to use a slice (ptr[0..len]), which has bounds checks.

exoego · 2024-06-22T09:08:46Z

Thanks!!

Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: dave caruso <[email protected]>

afonsojramos · 2024-07-04T23:39:12Z

src/cli/test_command.zig

+            const tmpname = std.fmt.bufPrintZ(&shortname_buf, ".lcov.info.{s}.tmp", .{bun.fmt.fmtSliceHexLower(&base64_bytes)}) catch unreachable;
+            const path = bun.path.joinAbsStringBufZ(relative_dir, &lcov_name_buf, &.{ opts.reports_directory, tmpname }, .auto);


Running multiple times is leaving the folder with multiple tml files, is this expected?

Add coverageReporters and coverageReportsDir

5eca3b2

exoego force-pushed the lcov branch from f292267 to 7e5e324 Compare June 15, 2024 00:32

exoego marked this pull request as ready for review June 15, 2024 05:18

Implement LCOV coverage report

7ac1af8

exoego force-pushed the lcov branch from 7e5e324 to 7ac1af8 Compare June 15, 2024 05:20

exoego changed the title ~~Implement LCOV reporter without function names supports~~ Implement initial LCOV reporter (no function names support) Jun 15, 2024

Create a reports dir recursively

b74d223

Electroid requested a review from Jarred-Sumner June 17, 2024 20:20

Jarred-Sumner and others added 2 commits June 17, 2024 21:13

Merge branch 'main' into lcov

9648ba9

Merge branch 'main' into lcov

9e75989

Merge remote-tracking branch 'origin/main' into lcov

b3a8eb7

paperclover requested changes Jun 21, 2024

View reviewed changes

exoego added 5 commits June 21, 2024 14:21

Remove unused iter

b3f7ae8

Avoid same reporter duplication

9ab1a77

Include invalid reporter in error log

a426f3e

Simplify with if-chain

19db90c

Reformat

0bf5f7e

exoego added 3 commits June 21, 2024 16:33

Added cli parameters

68d99da

Inline deref

54d12d3

Added snapshot test

75476db

exoego and others added 4 commits June 21, 2024 18:41

Merge branch 'main' into lcov

46cdd82

Added snapshot test

87e7c75

Merge remote-tracking branch 'origin/lcov' into lcov

ca7a3ed

rename "coverage report dir" option to just "coverage dir"

83b5375

paperclover approved these changes Jun 22, 2024

View reviewed changes

paperclover requested changes Jun 22, 2024

View reviewed changes

Jarred-Sumner added 2 commits June 22, 2024 00:36

Fix the undefined memory access, fix the out of bounds read & write, …

7bd86b1

…make it one pass, use more Windows-friendly APIs for creating the directory

Update coverage.test.ts.snap

daedfa3

Jarred-Sumner reviewed Jun 22, 2024

View reviewed changes

Make the test slightly more complete

3990f58

Jarred-Sumner merged commit 4830e2d into oven-sh:main Jun 22, 2024
49 of 53 checks passed

exoego deleted the lcov branch June 22, 2024 09:08

zackradisic pushed a commit that referenced this pull request Jun 24, 2024

Implement initial LCOV reporter (no function names support) (#11883)

da32326

Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: dave caruso <[email protected]>

dylan-conway pushed a commit that referenced this pull request Jun 26, 2024

Implement initial LCOV reporter (no function names support) (#11883)

a4744a4

Co-authored-by: Jarred Sumner <[email protected]> Co-authored-by: dave caruso <[email protected]>

afonsojramos reviewed Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement initial LCOV reporter (no function names support) #11883

Implement initial LCOV reporter (no function names support) #11883

exoego commented Jun 15, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024 •

edited

Loading

exoego commented Jun 21, 2024

paperclover left a comment

paperclover Jun 21, 2024

exoego Jun 21, 2024

paperclover Jun 21, 2024

exoego Jun 21, 2024

exoego commented Jun 21, 2024

paperclover commented Jun 21, 2024

exoego commented Jun 21, 2024 •

edited

Loading

paperclover left a comment

paperclover left a comment

exoego commented Jun 22, 2024

Jarred-Sumner Jun 22, 2024

Jarred-Sumner Jun 22, 2024

Jarred-Sumner Jun 22, 2024

exoego commented Jun 22, 2024

afonsojramos Jul 4, 2024

		const tmpname = std.fmt.bufPrintZ(&shortname_buf, ".lcov.info.{s}.tmp", .{bun.fmt.fmtSliceHexLower(&base64_bytes)}) catch unreachable;
		const path = bun.path.joinAbsStringBufZ(relative_dir, &lcov_name_buf, &.{ opts.reports_directory, tmpname }, .auto);

Implement initial LCOV reporter (no function names support) #11883

Implement initial LCOV reporter (no function names support) #11883

Conversation

exoego commented Jun 15, 2024 • edited Loading

What does this PR do?

How did you verify your code works?

github-actions bot commented Jun 17, 2024 • edited Loading

💻 1 failing tests Darwin x64 baseline

🐧💪 1 failing tests Linux AARCH64

🪟💻 1 failing tests Windows x64 baseline

🪟💻 2 failing tests Windows x64

exoego commented Jun 21, 2024

paperclover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exoego commented Jun 21, 2024

paperclover commented Jun 21, 2024

exoego commented Jun 21, 2024 • edited Loading

paperclover left a comment

Choose a reason for hiding this comment

paperclover left a comment

Choose a reason for hiding this comment

exoego commented Jun 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exoego commented Jun 22, 2024

Choose a reason for hiding this comment

exoego commented Jun 15, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024 •

edited

Loading

exoego commented Jun 21, 2024 •

edited

Loading