Use configs

Author: lukaszcl

tools/flakeguard/cmd/run.go

@@ -152,9 +152,8 @@ var RunTestsCmd = &cobra.Command{
 			skipTests,
 			selectTests,
 			// Parser specific config
+			ignoreParentFailuresOnSubtests, // Pass parser config directly
 			omitOutputsOnSuccess,
-			passRatioThreshold, // Pass the calculated threshold
-			ignoreParentFailuresOnSubtests,
 			// Dependencies (pass nil to get defaults)
 			nil, // exec
 			nil, // parser

tools/flakeguard/runner/parser/parser.go

@@ -60,7 +60,7 @@ func (e entry) String() string {
 type Parser interface {
 	// ParseFiles takes a list of raw output file paths, processes them (including potential transformation),
 	// and returns the aggregated test results and the list of file paths that were actually parsed.
-	ParseFiles(rawFilePaths []string, runPrefix string, expectedRuns int, ignoreParentFailures bool, omitSuccessOutputs bool) ([]reports.TestResult, []string, error)
+	ParseFiles(rawFilePaths []string, runPrefix string, expectedRuns int, cfg Config) ([]reports.TestResult, []string, error)
 }
 
 // Config holds configuration relevant to the parser.
@@ -85,20 +85,20 @@ func NewParser() Parser {
 
 // ParseFiles is the main entry point for the parser.
 // It orchestrates transformation (if needed) and parsing of multiple files.
-func (p *defaultParser) ParseFiles(rawFilePaths []string, runPrefix string, expectedRuns int, ignoreParentFailures bool, omitSuccessOutputs bool) ([]reports.TestResult, []string, error) {
+func (p *defaultParser) ParseFiles(rawFilePaths []string, runPrefix string, expectedRuns int, cfg Config) ([]reports.TestResult, []string, error) {
 	var parseFilePaths = rawFilePaths
 
-	// If the option is enabled, transform each JSON output file before parsing.
-	if ignoreParentFailures {
+	// Use cfg for transformation decision
+	if cfg.IgnoreParentFailuresOnSubtests {
 		err := p.transformTestOutputFiles(rawFilePaths)
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed during output transformation: %w", err)
 		}
 		parseFilePaths = p.transformedOutputFiles
 	}
 
-	// Now parse the selected files (raw or transformed)
-	results, err := p.parseTestResults(parseFilePaths, runPrefix, expectedRuns, omitSuccessOutputs)
+	// Pass cfg down to parseTestResults
+	results, err := p.parseTestResults(parseFilePaths, runPrefix, expectedRuns, cfg)
 	if err != nil {
 		return nil, parseFilePaths, err // Return paths even on error?
 	}
@@ -109,7 +109,7 @@ func (p *defaultParser) ParseFiles(rawFilePaths []string, runPrefix string, expe
 // parseTestResults reads the test output Go test json output files and returns processed TestResults.
 // This is the core logic moved from the original Runner.parseTestResults.
 // It now takes file paths directly.
-func (p *defaultParser) parseTestResults(parseFilePaths []string, runPrefix string, expectedRuns int, omitSuccessOutputs bool) ([]reports.TestResult, error) {
+func (p *defaultParser) parseTestResults(parseFilePaths []string, runPrefix string, expectedRuns int, cfg Config) ([]reports.TestResult, error) {
 	var (
 		testDetails         = make(map[string]*reports.TestResult) // Holds run, pass counts, and other details for each test
 		panickedPackages    = map[string]struct{}{}                // Packages with tests that panicked
@@ -185,10 +185,8 @@ func (p *defaultParser) parseTestResults(parseFilePaths []string, runPrefix stri
 			if entryLine.Test != "" {
 				// If it's a subtest, associate it with its parent for easier processing of panics later
 				key := fmt.Sprintf("%s/%s", entryLine.Package, entryLine.Test)
-				// Call the utility function (assuming it's accessible, might need import alias or move)
-				// For now, assume it's callable directly if parser stays in runner package temporarily, or needs adjustment.
-				// Let's assume we need to call a utility function `parseSubTest`.
-				parentTestName, subTestName := parseSubTest(entryLine.Test) // This needs to resolve
+				// Call the utility function (now internal to this package)
+				parentTestName, subTestName := parseSubTest(entryLine.Test) // Use internal parseSubTest
 				if subTestName != "" {
 					parentTestKey := fmt.Sprintf("%s/%s", entryLine.Package, parentTestName)
 					// Ensure slice exists before appending
@@ -554,7 +552,7 @@ func (p *defaultParser) parseTestResults(parseFilePaths []string, runPrefix stri
 			result.PackageOutputs = outputs
 		}
 
-		if omitSuccessOutputs {
+		if cfg.OmitOutputsOnSuccess {
 			result.PassedOutputs = make(map[string][]string)
 			result.Outputs = make(map[string][]string)
 		}
@@ -617,12 +615,8 @@ func (p *defaultParser) transformTestOutputFiles(filePaths []string) error {
 	return nil
 }
 
-// parseSubTest needs to be accessible here. It should be moved to a shared utility location
-// or this parser needs to be part of the runner package still.
-// Assuming it's moved to runner/utils.go and this file becomes part of the runner package
-// OR this file becomes parser package and imports runner (circular dependency risk) or utils.
-// Let's assume utils for now.
-// This function is currently duplicated - needs cleanup.
+// parseSubTest checks if a test name is a subtest and returns the parent and sub names.
+// Moved back into parser package and kept unexported.
 func parseSubTest(testName string) (parentTestName, subTestName string) {
 	parts := strings.SplitN(testName, "/", 2)
 	if len(parts) == 1 {

tools/flakeguard/runner/runner.go

@@ -11,24 +11,16 @@ import (
 	"github.com/smartcontractkit/chainlink-testing-framework/tools/flakeguard/reports"
 	"github.com/smartcontractkit/chainlink-testing-framework/tools/flakeguard/runner/executor"
 	"github.com/smartcontractkit/chainlink-testing-framework/tools/flakeguard/runner/parser"
-	// No parser import needed if files are in the same package
 )
 
 const (
 	RawOutputDir = "./flakeguard_raw_output"
-	// Removed RawOutputTransformedDir - belongs in parser.go
-)
-
-var (
-// buildErr remains defined in the parser package
-// Consider defining shared errors in a common errors package if needed elsewhere
 )
 
 // Runner describes the test run parameters and manages test execution and result parsing.
-// It now delegates command execution to an Executor and result parsing to a Parser.
+// It delegates command execution to an Executor and result parsing to a Parser.
 type Runner struct {
-	// Configuration fields (consider grouping into a Config struct if it grows larger)
-	// Keep only fields relevant to Runner's orchestration role or executor config
+	// Configuration fields
 	ProjectPath       string
 	Verbose           bool
 	RunCount          int
@@ -42,10 +34,9 @@ type Runner struct {
 	SkipTests         []string
 	SelectTests       []string
 
-	// Configuration for the parser
-	OmitOutputsOnSuccess           bool
-	MaxPassRatio                   float64 // TODO: Is this still used by Runner or belongs elsewhere?
+	// Configuration passed down to the parser
 	IgnoreParentFailuresOnSubtests bool
+	OmitOutputsOnSuccess           bool
 
 	// Dependencies
 	exec   executor.Executor // Injected Executor
@@ -68,10 +59,9 @@ func NewRunner(
 	failFast bool,
 	skipTests []string,
 	selectTests []string,
-	// Parser specific config
+	// Parser specific config (passed during initialization)
+	ignoreParentFailuresOnSubtests bool,
 	omitOutputsOnSuccess bool,
-	maxPassRatio float64, // Note: MaxPassRatio currently unused after refactor, belongs in reporting?
-	ignoreParentFailuresOnSubtests bool, // Passed to parser
 	// Dependencies (allow injection for testing)
 	exec executor.Executor,
 	p parser.Parser, // Use interface type directly
@@ -95,9 +85,8 @@ func NewRunner(
 		FailFast:                       failFast,
 		SkipTests:                      skipTests,
 		SelectTests:                    selectTests,
-		OmitOutputsOnSuccess:           omitOutputsOnSuccess,
-		MaxPassRatio:                   maxPassRatio,
 		IgnoreParentFailuresOnSubtests: ignoreParentFailuresOnSubtests,
+		OmitOutputsOnSuccess:           omitOutputsOnSuccess,
 		exec:                           exec,
 		parser:                         p,
 	}
@@ -120,6 +109,14 @@ func (r *Runner) getExecutorConfig() executor.Config {
 	}
 }
 
+// Helper function to create parser.Config from Runner fields
+func (r *Runner) getParserConfig() parser.Config {
+	return parser.Config{
+		IgnoreParentFailuresOnSubtests: r.IgnoreParentFailuresOnSubtests,
+		OmitOutputsOnSuccess:           r.OmitOutputsOnSuccess,
+	}
+}
+
 // RunTestPackages executes the tests for each provided package and aggregates all results.
 func (r *Runner) RunTestPackages(packages []string) ([]reports.TestResult, error) {
 	rawOutputFiles := make([]string, 0) // Collect output file paths for this run
@@ -151,12 +148,12 @@ ParseResults:
 	}
 
 	log.Info().Int("file_count", len(rawOutputFiles)).Msg("Parsing output files")
+	// Create parser config and pass it
+	parserCfg := r.getParserConfig()
 	// Ignore the returned file paths here, as they aren't used in this flow
-	results, _, err := r.parser.ParseFiles(rawOutputFiles, "run", len(rawOutputFiles), r.IgnoreParentFailuresOnSubtests, r.OmitOutputsOnSuccess)
+	results, _, err := r.parser.ParseFiles(rawOutputFiles, "run", len(rawOutputFiles), parserCfg)
 	if err != nil {
 		// Check if it's a build error from the parser
-		// Use errors.Is with the error defined in the parser package (or a shared error package)
-		// Assuming parser.ErrBuild is exported:
 		if errors.Is(err, parser.ErrBuild) { // Updated check
 			// No extra wrapping needed if buildErr already provides enough context
 			return nil, err
@@ -195,8 +192,10 @@ func (r *Runner) RunTestCmd(testCmd []string) ([]reports.TestResult, error) {
 	}
 
 	log.Info().Int("file_count", len(rawOutputFiles)).Msg("Parsing output files from custom command")
+	// Create parser config and pass it
+	parserCfg := r.getParserConfig()
 	// Ignore the returned file paths here as well
-	results, _, err := r.parser.ParseFiles(rawOutputFiles, "run", len(rawOutputFiles), r.IgnoreParentFailuresOnSubtests, r.OmitOutputsOnSuccess)
+	results, _, err := r.parser.ParseFiles(rawOutputFiles, "run", len(rawOutputFiles), parserCfg)
 	if err != nil {
 		if errors.Is(err, parser.ErrBuild) { // Updated check
 			return nil, err
@@ -287,19 +286,24 @@ func (r *Runner) RerunFailedTests(failedTests []reports.TestResult, rerunCount i
 	}
 
 	log.Info().Int("file_count", len(rerunOutputFiles)).Msg("Parsing rerun output files")
+	// Create parser config and pass it
+	parserCfg := r.getParserConfig()
 	// For parsing reruns, the effective number of runs *per test included in the output* is `rerunCount`.
 	// The parser's `expectedRuns` helps adjust for potential overcounting within each file, using `rerunCount` seems correct here.
-	rerunResults, parsedFilePaths, err := r.parser.ParseFiles(rerunOutputFiles, "rerun", rerunCount, r.IgnoreParentFailuresOnSubtests, r.OmitOutputsOnSuccess)
+	rerunResults, parsedFilePaths, err := r.parser.ParseFiles(rerunOutputFiles, "rerun", rerunCount, parserCfg)
 	if err != nil {
 		// Check for build error specifically?
 		if errors.Is(err, parser.ErrBuild) { // Updated check
 			log.Error().Err(err).Msg("Build error occurred unexpectedly during test reruns")
 			// Fallthrough to return wrapped error
 		}
+		// Return the file paths even if parsing failed? No, the report wouldn't be useful.
 		return nil, nil, fmt.Errorf("failed to parse rerun results: %w", err)
 	}
 
 	// 5. Return Results
 	log.Info().Int("result_count", len(rerunResults)).Msg("Finished parsing rerun results")
+	// Return the parsed results AND the list of files parsed.
+	// Note: The function signature needs to change back to return []string
 	return rerunResults, parsedFilePaths, nil
 }