Skip to content

Adding unique keys for keeping track of scenarios in rerun report. - #2880

Open
jensakejohansson wants to merge 4 commits into
getgauge:masterfrom
system-verification:issue/2871
Open

Adding unique keys for keeping track of scenarios in rerun report.#2880
jensakejohansson wants to merge 4 commits into
getgauge:masterfrom
system-verification:issue/2871

Conversation

@jensakejohansson

Copy link
Copy Markdown
Contributor

Proposed fix for issue: #2871

Problem

With a data-table-driven scenario, Gauge emits a separate ScenarioEnd event for each table row, but they all share the same file:line key because they're the same scenario definition. So with a 3-row table where row 1 passes, row 2 fails, row 3 passes:

  1. Row 1 passes -> removeFailedItem("example.spec:13") -> no-op (nothing stored yet)
  2. Row 2 fails -> addFailedItem("example.spec:13") -> failure recorded
  3. Row 3 passes -> removeFailedItem("example.spec:13") -> incorrectly removes row 2's failure

Result: failures.json has no entry despite the scenario genuinely failing for row 2. Rerunning failed tests does nothing.

Fix

Two-step approach:

  1. Internal tracking uses precise keys. scenarioFailureRef now builds keys that include table row indices (e.g., example.spec:13:2 for spec-level table row index 2. This means row 3 passing calls removeFailedItem("example.spec:13:3"), which doesn't match row 2's key example.spec:13:2, so the failure is preserved.

  2. Output strips back to file:line format. scenarioFailureRefForOutput reduces example.spec:13:2 back to example.spec:13 when building FailedItems for failures.json, because Gauge's parser expects the file:line format. Deduplication ensures multiple failed rows produce a single entry.

Note, nested spec and scenario tables are supported resulting in internal keys like example.spec:13:2:1

Tests

I've added a number of tests for regression.

Signed-off-by: jensakejohansson <jens.johansson@systemverification.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds row-aware rerun tracking so passing table rows do not erase failures from other rows.

Changes:

  • Adds spec/scenario table-row indices to internal failure keys.
  • Normalizes and deduplicates keys before writing failures.json.
  • Adds regression tests for table-driven scenarios and retries.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
execution/rerun/rerun.go Implements row-aware tracking and output normalization.
execution/rerun/rerun_test.go Tests row isolation, retries, nested keys, and deduplication.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread execution/rerun/rerun.go Outdated
Comment thread execution/rerun/rerun.go Outdated
@github-actions

This comment was marked as outdated.

…AUGE_SPEC_FILE_EXTENSIONS.

Signed-off-by: jensakejohansson <jens.johansson@systemverification.com>
@jensakejohansson

This comment was marked as resolved.

chadlwilson

This comment was marked as resolved.

@jensakejohansson

This comment was marked as resolved.

Signed-off-by: jensakejohansson <jens.johansson@systemverification.com>
…imply logic in rerun.

Signed-off-by: jensakejohansson <jens.johansson@systemverification.com>
@jensakejohansson

jensakejohansson commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Now there is a struct to use as key instead. Also added HasSpecDataTable bool to gauge.Scenario, set by the parser, so it can be checked in the logic in the key generation. Makes it less confusing than the OR sce.SpecDataTableRow.IsInitialized() || sce.ScenarioDataTableRow.IsInitialized() needed otherwise to achieve the same result / unique keys.

Any thoughts? Is along the lines of what you intended?

@chadlwilson

Copy link
Copy Markdown
Contributor

Something like that, yeah. Need to look with the wider code context later probably.

The booleans are ugly and create another failure mode it seems, but if following existing patterns... ok. Given annoyances of hashing pointers, maybe better to just use -1 sentinel values via constructor function or otherwise we have yet more combination of bool+int to reconcile (and there are seemingly many bugs, e.g that trying to be addressed by #2879)

This comment was marked as resolved.

@jensakejohansson

Copy link
Copy Markdown
Contributor Author

Something like that, yeah. Need to look with the wider code context later probably.

The booleans are ugly and create another failure mode it seems, but if following existing patterns... ok. Given annoyances of hashing pointers, maybe better to just use -1 sentinel values via constructor function or otherwise we have yet more combination of bool+int to reconcile (and there are seemingly many bugs, e.g that trying to be addressed by #2879)

I agree there are may parameters floating around, sometimes with unclear names too, but I tried my best with what there is. Especially when working with code related to table driven execution I'd like to touch as little as possible. Which is a bad sign. If I understand you correctly I can leave it as is for now, and you might check more details when you have time... thanks for your feedback.

P.S
I have no hard facts, but I always get the feeling that there is quite a lot of things that could be streamlined, simplified and optimized. I just don't dare to attempt any larger changes and I don't really have the time either. Otherwise I would opt for a "Gauge 2.0-beta" stream with the aim to refactor the scenario parsing/execution parts, more efficient memory management, unify the different reporting outputs etc, etc.

@chadlwilson

Copy link
Copy Markdown
Contributor

Yeah, I'm similar. And many areas you probably know the code better than me.

I just like to chip away at the bits that seem to be more buggy and attracting attention so they are incrementally easier to review and reason about such that it reduces the maintenance effort. A change that fixes something is better than no change, as long as we can reasonably conclude its unlikely to break things, or make existing problems worse.

I'm mainly here because GoCD uses Gauge Ruby and I resurrected its language plugin, and as a (now ex) TWer I knew some of the Gauge devs indirectly, many of whom used to work on GoCD.

However I/GoCD don't use table driven anything and its specs are not even super well factored to good Gauge practices, so there are many areas I dont have solid intuitions about. Thus I focus on stability to allow me to keep things patched across the many plugins that need to be reasonably forward and backward compatible, released securely etc, and so am not the right maintainer for more invasive changes.

The original maintainers probably have/had a slightly different philosophy to me - one more of "merge first, ask questions later", but to me that's a luxury of s project with a healthy maintainer pool (to review and release follow ups) which Gauge certainly is not in 2026 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants