Add feature to collect function/statement/branch coverage to `test` command #343

nodaguti · 2024-07-28T11:05:46Z

This PR implements a new feature to collect test coverage data while running tests with -c / --coverage option enabled and report them in the output (either in human-friendly way or in JSON).

Approach

There are roughly two ways to implement coverage measurement: 1) collecting coverage in the interpreter side (like V8 JavaScript code coverage), and 2) collecting coverage in the userland side (like Istanbul test coverage)

For option 1, adding support for code coverage to our interpreter may result in tight coupling between the interpreter and coverage measurement, which I think would not a great idea especially when considering a fact that falco uses the interpreter not only in tester but also in simulator and console features.

For option 2, since we already have a special function to mutate an existing table in tester mode, testing.table_set, tables can be used to store coverage data.

This PR adds a new process called "instrumentation" to the preparation phase of testing: traversing an AST tree to find subroutines, statements, and branches to initialise coverage tables, and injecting testing.table_set statements to record executions of each subroutine, statement, or branch.

These injected statements will be evaluated along with original VCL code and produce coverage information for each test case.

After running all tests, each coverage information will be merged so that falco can calculate overall coverage percentages.

Screenshot

Related Issues

[Feature] VCL analayzer/optimizer #314

nodaguti · 2024-07-28T11:06:51Z

@ysugimoto
I'm sorry for the PR being relatively large, but I look forward to hearing your impressions. Thank you!

ysugimoto

This PR seems to be conceptual and looks very cool approach.

I put some request changes for adjusting current implementation, and this will be needed more working so could you change the base branch to the develop?
I've just created develop branch from current main branch, let's do more working on it.

ysugimoto · 2024-07-30T05:49:33Z

config/config.go

@@ -62,6 +62,7 @@ type SimulatorConfig struct {
 type TestConfig struct {
 	Timeout      int      `cli:"t,timeout" yaml:"timeout"`
 	Filter       string   `cli:"f,filter" default:"*.test.vcl"`
+	Coverage     bool     `cli:"c,coverage" yaml:"coverage" default:"false"`


Sorry for the our circumstance, could the coverage option be a --converage - only long option?
This is because we'd like to use -c short option for the other usage like config path definition.

ysugimoto · 2024-07-30T05:50:29Z

docs/configuration.md

@@ -65,6 +65,7 @@ All configurations of configuration files and CLI arguments are described follow
 | simulator.edge_dictionary.[name]   | Object        | -       | -                  | Local edge dictionary name                                                                                                            |
 | testing                            | Object        | null    | -                  | Testing configuration object                                                                                                          |
 | testing.timeout                    | Integer       | 10      | -t, --timeout      | Set timeout to stop testing                                                                                                           |
+| testing.coverage                   | Boolean       | false   | -c, --coverage     | Collect test coverage information and report it in the output                                                                         |


Sorry for the our circumstance, could the coverage option be a --converage - only long option?
This is because we'd like to use -c short option for the other usage like config path definition.

ysugimoto · 2024-07-30T05:53:27Z

interpreter/coverage.go

+func getFunctionId(s ast.SubroutineDeclaration) string {
+	return s.GetMeta().Token.File + "_" + s.Name.Value
+}
+
+func getStatementId(stmt ast.Statement) string {
+	t := stmt.GetMeta().Token
+	l := strconv.Itoa(t.Line)
+	p := strconv.Itoa(t.Position)
+	return t.File + "_stmt_l" + l + "_p" + p
+}
+
+func getExpressionId(exp ast.Expression) string {
+	t := exp.GetMeta().Token
+	l := strconv.Itoa(t.Line)
+	p := strconv.Itoa(t.Position)
+	return t.File + "_exp_l" + l + "_p" + p
+}


The parser assigns unique id for each node so the id would be useful for recognizing function, statement, and expression. What do you think about it?
ref: https://github.com/ysugimoto/falco/blob/main/ast/ast.go#L79-L87

ysugimoto · 2024-07-30T05:57:18Z

interpreter/coverage.go

+	i.ctx.Tables[FUNCTION_COVERAGE] = &ast.TableDeclaration{
+		Meta:       &ast.Meta{Token: token.Null},
+		Name:       &ast.Ident{Value: FUNCTION_COVERAGE},
+		ValueType:  &ast.Ident{Value: "STRING"},
+		Properties: []*ast.TableProperty{},
+	}
+	i.ctx.Tables[STATEMENT_COVERAGE] = &ast.TableDeclaration{
+		Meta:       &ast.Meta{Token: token.Null},
+		Name:       &ast.Ident{Value: STATEMENT_COVERAGE},
+		ValueType:  &ast.Ident{Value: "STRING"},
+		Properties: []*ast.TableProperty{},
+	}
+	i.ctx.Tables[BRANCH_COVERAGE] = &ast.TableDeclaration{
+		Meta:       &ast.Meta{Token: token.Null},
+		Name:       &ast.Ident{Value: BRANCH_COVERAGE},
+		ValueType:  &ast.Ident{Value: "STRING"},
+		Properties: []*ast.TableProperty{},
+	}


i.ctx.Tables holds the actual table declarations that are defined in a VCL so rarely it may conflict hence we should avoid it.
We could declare another property that dedicated to hold the converage state in the context.Context struct here instead. And, the data type could be arbitrary so you don't need to define as a table declaration. I think it's good enough to declare like:

type CoverageInfo struct { Functions map[string]ast.Node Statements map[string]ast.Node Branches map[string]ast.Node }

The map value should be ast.Node interface because it will be useful for generating converage report against the actual VCL.

ysugimoto · 2024-07-30T07:08:03Z

interpreter/coverage.go

+		[]ast.Statement{
+			createMarkAsCovered(FUNCTION_COVERAGE, id),
+		},


This seems like an instanbul approach, interesting and very nice!

ysugimoto · 2024-07-30T07:17:42Z

interpreter/coverage.go

+type Coverage struct {
+	Function  CoverageTable
+	Statement CoverageTable
+	Branch    CoverageTable
+}
+
+type CoverageTable map[string]bool
+
+func (i *Interpreter) GetCoverage() Coverage {
+	if i.ctx.Tables[FUNCTION_COVERAGE] == nil || i.ctx.Tables[STATEMENT_COVERAGE] == nil || i.ctx.Tables[BRANCH_COVERAGE] == nil {
+		return Coverage{}
+	}
+
+	return Coverage{
+		Function:  convertToCoverageTable(i.ctx.Tables[FUNCTION_COVERAGE]),
+		Statement: convertToCoverageTable(i.ctx.Tables[STATEMENT_COVERAGE]),
+		Branch:    convertToCoverageTable(i.ctx.Tables[BRANCH_COVERAGE]),
+	}
+}
+
+func convertToCoverageTable(decl *ast.TableDeclaration) CoverageTable {
+	table := make(CoverageTable)
+
+	for _, prop := range decl.Properties {
+		table[prop.Key.Value] = prop.Value.(*ast.String).Value == "true"
+	}
+
+	return table
+}


Probably you can declare this types in the interpreter's context package.

ysugimoto · 2024-07-30T07:21:49Z

interpreter/coverage.go

+		Meta: &ast.Meta{Token: token.Null},
+		Function: &ast.Ident{
+			Meta: &ast.Meta{
+				Token: token.Token{Type: token.STRING, Literal: "testing.table_set"},


I suggest that you can create new testing function for marking converage like coverage.mark() but it's okay for this PR. we can do it later!

ysugimoto · 2024-07-30T07:28:09Z

interpreter/coverage_test.go

+			instrumented: `
+sub func1 {
+  testing.table_set(falco_coverage_function, "main_func1", "true");
+  testing.table_set(falco_coverage_statement, "main_stmt_l3_p3", "true");
+  if (req.http.Foo == "1") {
+    testing.table_set(falco_coverage_branch, "main_exp_l3_p13_true", "true");
+  } else {
+    testing.table_set(falco_coverage_branch, "main_exp_l3_p13_false", "true");
+  }
+  error 600 if(req.http.Foo == "1", "1", "2");
+  testing.table_set(falco_coverage_statement, "main_stmt_l4_p3", "true");
+  if (req.http.Foo == "1") {
+    testing.table_set(falco_coverage_branch, "main_exp_l4_p26_true", "true");
+  } else {
+    testing.table_set(falco_coverage_branch, "main_exp_l4_p26_false", "true");
+  }
+  header.set(req, "bar", if(req.http.Foo == "1", "1", "2"));
+  testing.table_set(falco_coverage_statement, "main_stmt_l5_p3", "true");
+  if (req.http.Foo == "1") {
+    testing.table_set(falco_coverage_branch, "main_exp_l5_p22_true", "true");
+  } else {
+    testing.table_set(falco_coverage_branch, "main_exp_l5_p22_false", "true");
+    if (req.http.Foo == "2") {
+      testing.table_set(falco_coverage_branch, "main_exp_l5_p60_true", "true");
+    } else {
+      testing.table_set(falco_coverage_branch, "main_exp_l5_p60_false", "true");
+    }
+  }
+  set req.http.Bar = if(req.http.Foo == "1", "1", "bar_" + if(req.http.Foo == "2", "2", "3"));
+}


This looks tricky a bit but very interesting, awesome!

ysugimoto · 2024-07-30T07:29:13Z

interpreter/testing.go

 	icontext "github.com/ysugimoto/falco/interpreter/context"
 	"github.com/ysugimoto/falco/interpreter/value"
 )

 const testBackendResponseBody = "falco_test_response"

-func (i *Interpreter) TestProcessInit(r *http.Request) error {
+func (i *Interpreter) TestProcessInit(r *http.Request, c *config.TestConfig) error {


Passing a bool argument like enableCoverage bool seems to be enough.

ysugimoto · 2025-01-28T10:36:29Z

@nodaguti Will you continue to work on this PR?
I think basically Okay so will merge into main and refactor it on an another PR.

nodaguti added 4 commits July 28, 2024 18:55

Add -c / --coverage option to switch coverage measurement

7ae1237

Instrument code to measure coverage

5a3b16b

Calculate coverage after running all tests

ed62b6e

Add tests

1d062a7

ysugimoto requested changes Jul 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature to collect function/statement/branch coverage to `test` command #343

Add feature to collect function/statement/branch coverage to `test` command #343

nodaguti commented Jul 28, 2024 •

edited

Loading

nodaguti commented Jul 28, 2024

ysugimoto left a comment

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto Jul 30, 2024

ysugimoto commented Jan 28, 2025

Add feature to collect function/statement/branch coverage to test command #343

Are you sure you want to change the base?

Add feature to collect function/statement/branch coverage to test command #343

Conversation

nodaguti commented Jul 28, 2024 • edited Loading

Approach

Screenshot

Related Issues

nodaguti commented Jul 28, 2024

ysugimoto 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

ysugimoto commented Jan 28, 2025

Add feature to collect function/statement/branch coverage to `test` command #343

Add feature to collect function/statement/branch coverage to `test` command #343

nodaguti commented Jul 28, 2024 •

edited

Loading