feat(gnovm): Test Coverage Support #2616

notJoon · 2024-07-23T08:14:42Z

Description

Support testing coverage functionality into previous testing framework. This feature tracks and reports which parts of the code were executed during test runs.

Key Changes

Added coverage data structures
- Implemented CoverageData and FileCoverage structs
- Per-file executed line tracker
Coverage collection machanism
- Collect current location information after PopOp in Machine.Run() method
  - Inserted recordCoverage function for each statement to obtaining accurate coverage tracking result
- Record executed lines throught AddHit method
Coverage reporting
- Calculate and output coverage percentage per file
  - Excludes non-executable lines (e.g., comments) from the calculation
- Terminal visualization using ANSI color codes
Connect with external tools
- Convert coverage data to JSON format

CLI Options

Flag	Desc	Example
`-cover`	Activate coverage analysis	`gno test <path> -cover`
`-view=<pattern>`	View coverage for files matching the pattern	`gno test <path> -cover -view=demo/foo`
`-show-hits`	Show number of times each line was executed	`gno test <path> -cover -view=demo/foo -show-hits`
`-html=<file>`	Generate HTML coverage report	`gno test <path> -cover -html=coverage.html`

All coverage-related flags must be preceded by the -cover flag to be active.

Notes:

The report displays a list of files with their coverage percentages, color-coded based on coverage level.
The -view option supports partial file path patterns and can match multiple files.
When using -view, the output is color-coded: green for executed lines, yellow for executable but not executed lines, and white for non-executable lines.
The -show-hits option adds an orange hit count next to each executed line when used with -view.

The currently added CLI was added for testing during development. I plan to modify it later to provide the same functionality as go tool.

codecov · 2024-07-23T08:17:54Z

Codecov Report

Attention: Patch coverage is 55.76923% with 207 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gnovm/pkg/coverage/report.go	64.58%	55 Missing and 13 partials ⚠️
gnovm/pkg/gnolang/machine.go	14.28%	64 Missing and 2 partials ⚠️
gnovm/pkg/coverage/coverage.go	57.14%	34 Missing and 2 partials ⚠️
gnovm/pkg/coverage/analyze.go	56.33%	30 Missing and 1 partial ⚠️
gnovm/pkg/coverage/utils.go	84.61%	4 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

….go and op_eval.go

notJoon · 2024-09-12T10:16:18Z

For simplicity for reviewing I'll change this PR as ready for review.

notJoon · 2024-10-22T02:50:53Z

@zivkovicmilos @thehowl

Now all CI passes successfully. Previously, I had set it up to always generate and store coverage data whenever tests were run. But this caused timeouts in the strings and bytes packages due to increase overhead (#2935). So I modified it to collect data only when the coverage flag (-flag) is activated.

Currently, since it only perfoems basic operations, there are cases where lines that should be executed are not recognized during the execution. However, this case requires branch coverage and other static analysis features to be implemented. So, I plan to make these in a separate PR to prevent the current PR size from becoming too large.

The planned tasks are as follows:

Run static analysis based on the collected line information to calculate more accurate coverage
Provide the same commands and functionalities as go tool cover (ref: gno test -cover ... #1121)

jefft0 · 2024-10-25T07:53:19Z

Hello @notJoon . It says "This branch has conflicts that must be resolved". Are there merge conflicts with master?

notJoon · 2024-10-25T14:46:47Z

Hello @notJoon . It says "This branch has conflicts that must be resolved". Are there merge conflicts with master?

@jefft0 now fixed

jefft0 · 2024-10-25T14:54:03Z

The changes look reasonable to me, but we need a core dev to decide if they are needed. Do you need anything else from me for the "triage" review?

notJoon · 2024-10-28T05:58:04Z

The changes look reasonable to me, but we need a core dev to decide if they are needed. Do you need anything else from me for the "triage" review?

@jefft0

I think checking just the basic things should be sufficient. I've added flags as per convention.

jefft0 · 2024-11-14T09:47:52Z

@notJoon , can you resolve the conflicts with the master branch?

notJoon · 2024-11-14T10:58:25Z

@jefft0 resolved. thanks 👍

jefft0 · 2024-11-15T12:45:06Z

@notJoon, the failing CI checks are because of a misconfiguration of codecov. This has been fixed. Can you please push an empty commit? This will make the CI checks run again and hopefully pass.

zivkovicmilos

I like the feature, but I think we need to change some things about the implementation 🙏

I'm primarily concerned that this sort of functionality and code doesn't belong in pkg/gnolang, but warrants its own separate package. Please check the comments, and we can take it from there

cc @thehowl

zivkovicmilos · 2024-11-20T03:08:37Z

gnovm/cmd/gno/test.go

@@ -257,24 +301,33 @@ func gnoTestPkg(
 		stdout = commands.WriteNopCloser(mockOut)
 	}

+	coverageData := gno.NewCoverageData(cfg.rootDir)


Nitpick:

coverageData := gno.NewCoverageData(cfg.rootDir).Enable() if !cfg.coverage { coverageData.Disable() }

zivkovicmilos · 2024-11-20T03:08:45Z

gnovm/cmd/gno/test.go

 	// testing with *_test.gno
 	if len(unittestFiles) > 0 {
 		// Determine gnoPkgPath by reading gno.mod
 		var gnoPkgPath string
 		modfile, err := gnomod.ParseAt(pkgPath)
 		if err == nil {
+			// TODO: use pkgPathFromRootDir?


zivkovicmilos · 2024-11-20T03:09:31Z

gnovm/cmd/gno/test.go

@@ -392,6 +450,36 @@ func gnoTestPkg(
 		}
 	}

+	if cfg.coverage {


Nitpick:

if !coverageData.IsEnabled() {return errs} // ...

zivkovicmilos · 2024-11-20T03:10:32Z

gnovm/pkg/gnolang/op_exec.go

@@ -433,6 +433,7 @@ EXEC_SWITCH:
 	}
 	switch cs := s.(type) {
 	case *AssignStmt:
+		m.recordCoverage(cs)


What is the overhead of something like this, for VM ops?

This should have condition check also. I forgot

zivkovicmilos · 2024-11-20T03:11:06Z

gnovm/pkg/gnolang/machine.go

@@ -81,6 +82,9 @@ type Machine struct {
 	// it is executed. It is reset to zero after the defer functions in the current
 	// scope have finished executing.
 	DeferPanicScope uint
+
+	// Test Coverage
+	Coverage *CoverageData


Does the VM usually keep track of this sort of data? cc @mvertes

I’ve added a this field to update executed lines directly on the Machine, but it might be better to separate this functionality if possible.

Maybe I can fetch the executed lines separately using a getter, and then handle them within the coverage package. This would eliminate the dependency and overhead on the Machine as well.

zivkovicmilos · 2024-11-20T03:27:33Z

gnovm/pkg/gnolang/coverage.go

+		strings.HasSuffix(pkgPath, "_filetest.gno")
+}
+
+type jsonCoverageMap map[string]JSONFileCoverage


You should move these types to the ToJSON method, and not have them be package-level types

zivkovicmilos · 2024-11-20T03:27:50Z

gnovm/pkg/gnolang/coverage.go

+	return json.MarshalIndent(jsonCov, "", "  ")
+}
+
+func (c *CoverageData) SaveJSON(fileName string) error {


This doesn't need to be a method on CoverageData

zivkovicmilos · 2024-11-20T03:28:08Z

gnovm/pkg/gnolang/coverage.go

+	return os.WriteFile(fileName, data, 0o644)
+}
+
+// TODO: temporary HTML output. need to match with go coverage tool


zivkovicmilos · 2024-11-20T03:28:18Z

gnovm/pkg/gnolang/coverage.go

+}
+
+// TODO: temporary HTML output. need to match with go coverage tool
+func (c *CoverageData) SaveHTML(outputFileName string) error {


This doesn't need to be a method on CoverageData

zivkovicmilos · 2024-11-20T03:28:48Z

gnovm/pkg/gnolang/coverage.go

+	return t.Execute(file, data)
+}
+
+func (m *Machine) addFileToCodeCoverage(file string, totalLines int) {


The Machine doesn't need this method, function can simply take in CoverageData

Gno2D2 · 2024-11-29T07:18:45Z

Merge Requirements

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🟢 Maintainers must be able to edit this pull request
🔴 The pull request head branch must be up-to-date with its base

Details

Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Head branch (notJoon:coverage) is up to date with base (master): behind by 8 / ahead by 44

Manual Checks

The pull request description provides enough details

Details

The pull request description provides enough details
If
🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
Can be checked by

team core-contributors

notJoon added 2 commits July 23, 2024 17:10

basic statement coverage

ccf344c

add control flow graph

78226e2

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 23, 2024

github-actions bot assigned notJoon Jul 23, 2024

notJoon added 20 commits July 23, 2024 19:00

wip: branch

509d36b

fix branch test

9f5e551

use cfg later. check more conditions

9a06318

revert

f9e44ce

Merge branch 'master' into coverage

225aac9

coverage type

fea3044

something works

b43bd02

save

6cbd004

save

7ec4d19

update logic

4295d5f

fix: file line handler

6aa2cfd

inspect only the package that contains the executed test file

c5854d7

feat: more precise coverage and add colored output command

b8f9e0c

Merge branch 'master' into coverage

c16d806

json output

6a458ba

Record coverage for variable assignments and expressions in op_assign…

38e7be4

….go and op_eval.go

calculate precise executable lines

75431a0

Refactor coverage data handling in Machine

12fbad6

remove unnecessat recodings

0870a7b

update CLI

ecd76ba

dongwon8247 mentioned this pull request Sep 11, 2024

Onbloc's Builder Journey gnolang/hackerspace#29

Open

fix minor bugs

8a35fc6

notJoon marked this pull request as ready for review September 12, 2024 10:16

notJoon requested a review from jaekwon as a code owner September 12, 2024 10:16

notJoon added 3 commits October 21, 2024 17:02

disable recording when not enabled

6ac0426

fix failed test

b3a0413

fix

cc0b24a

notJoon added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 22, 2024

notJoon added 3 commits October 25, 2024 18:35

Merge branch 'master' into coverage

e7c0d52

fix

0d06a6f

fix

c59c4a7

notJoon added 2 commits October 28, 2024 11:05

Merge branch 'master' into coverage

ae248da

fix

56cc61f

Merge branch 'master' into coverage

d2fac1d

fix lint error

003b798

Kouteki requested review from zivkovicmilos and petar-dambovaliev and removed request for jaekwon, moul, deelawn and piux2 November 18, 2024 06:49

zivkovicmilos requested changes Nov 20, 2024

View reviewed changes

Kouteki removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 20, 2024

notJoon added 2 commits November 27, 2024 17:46

Merge branch 'master' into coverage

abdd0b3

decouple

88128a4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnovm): Test Coverage Support #2616

feat(gnovm): Test Coverage Support #2616

notJoon commented Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

notJoon commented Sep 12, 2024

notJoon commented Oct 22, 2024 •

edited

Loading

jefft0 commented Oct 25, 2024

notJoon commented Oct 25, 2024 •

edited

Loading

jefft0 commented Oct 25, 2024

notJoon commented Oct 28, 2024

jefft0 commented Nov 14, 2024

notJoon commented Nov 14, 2024

jefft0 commented Nov 15, 2024

zivkovicmilos left a comment

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

notJoon Nov 21, 2024

zivkovicmilos Nov 20, 2024

notJoon Nov 21, 2024

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

zivkovicmilos Nov 20, 2024

Gno2D2 commented Nov 29, 2024 •

edited

Loading

If

Then

If

Then

If

Can be checked by

feat(gnovm): Test Coverage Support #2616

Are you sure you want to change the base?

feat(gnovm): Test Coverage Support #2616

Conversation

notJoon commented Jul 23, 2024 • edited Loading

Description

Key Changes

CLI Options

CLI Options

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

notJoon commented Sep 12, 2024

notJoon commented Oct 22, 2024 • edited Loading

jefft0 commented Oct 25, 2024

notJoon commented Oct 25, 2024 • edited Loading

jefft0 commented Oct 25, 2024

notJoon commented Oct 28, 2024

jefft0 commented Nov 14, 2024

notJoon commented Nov 14, 2024

jefft0 commented Nov 15, 2024

zivkovicmilos 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gno2D2 commented Nov 29, 2024 • edited Loading

Merge Requirements

Automated Checks

If

Then

If

Then

Manual Checks

If

Can be checked by

notJoon commented Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

notJoon commented Oct 22, 2024 •

edited

Loading

notJoon commented Oct 25, 2024 •

edited

Loading

Gno2D2 commented Nov 29, 2024 •

edited

Loading