Add e2e test cases for error categorization

[dejanzele]

What type of PR is this?

Feature (PR 4 of 4)

What this PR does / why we need it

Adds end-to-end test cases that verify error categories flow through the full Armada pipeline, asserting directly from the public event stream.

• Extends assertEventFailed in the testsuite event watcher to compare expected vs actual categories on JobFailedEvent (sorted exact match)
• Adds 2 categorization test cases: OOM kill (dd fills tmpfs to trigger OOMKilled) and user_error (exit code 1)
• Test YAML declares expected categories inline on the failed event:

  expectedEvents:
    - submitted:
    - failed:
        categories: ["oom"]

Which issue(s) this PR fixes

Part of #4713 (Error Categorization)

Special notes for your reviewer

• This is PR 4 of 4: Proto + classifier (Add error categorization proto schema and executor classifier #4741) -> Wire into executor (Wire error categories into executor event reporting #4745) -> Store in lookout DB + UI + public event (Store FailureInfo in lookout DB and expose via API #4755) -> e2e tests (this)
• Depends on Store FailureInfo in lookout DB and expose via API #4755 (which added categories field to JobFailedEvent)
• Categories are asserted from the event stream, not from the Lookout API - no HTTP polling or additional infrastructure needed
• CI executor config needs errorCategories rules added separately (included in e2e/config/executor_config.yaml)
• Both tests validated locally against a full local dev stack

[greptile-apps]

Greptile Summary

This PR adds end-to-end test cases for the error categorization feature (the final PR in a 4-part series), verifying that OOM kills and user errors flow through the full Armada pipeline and appear as the correct categories on the public JobFailedEvent stream. It also fixes a pre-existing logic inversion bug in assertEventFailed (the old code returned an error when the failure reason did match the regex, i.e. if re.MatchString(...) without !; the new code correctly errors when it does not match).

Key changes:

• validators.go: Refactors assertEventFailed to support category assertions with exact sorted-match semantics; fixes the long-standing regex logic inversion.
• app.go: Extends TestSpecsFromPattern to accept comma-separated glob patterns and updates the Prometheus failure-detection heuristic to recognise "expected categories" errors.
• magefiles/ci.go: Adds categorization/* to the CI test sweep.
• docker-compose.yaml + e2e/config/executor_config.yaml: Mounts a new executor config that activates the oom and user_error categorisation rules in docker-compose environments.
• Two new YAML test specs (oom.yaml, user_error.yaml) define the concrete scenarios.

Notable issues found:

• The assertCategories guard in validators.go silently skips the check when no expected categories are provided — there is no way to assert "this job should have zero categories" via the event stream path, leaving a coverage gap for future regression tests.
• TestSpecsFromPattern does not deduplicate file paths, so overlapping comma-separated patterns would run the same spec twice.
• The docker-compose.yaml change permanently enables the e2e errorCategories rules for all developers using the default dev stack, without a comment explaining the dependency.

Confidence Score: 3/5

• Safe to merge for CI coverage, but the validators.go category guard leaves a silent coverage gap that may weaken future regression tests.
• The two new test cases are correct and well-formed. The logic inversion fix in assertEventFailed is a genuine improvement. The main concern is the assertCategories guard that skips the check when expected categories are empty — this makes it impossible to assert zero categories via the event stream, and means existing basic/* tests provide no protection against future category-pollution bugs. The docker-compose change also broadly affects all developer environments without documentation. These are not blocking merge-safety issues, but they reduce the long-term reliability of the test suite.
• internal/testsuite/eventwatcher/validators.go — the assertCategories guard (line 41) and its interaction with tests that omit categories deserves attention before this pattern is extended further.

Important Files Changed

Filename	Overview
internal/testsuite/eventwatcher/validators.go	Refactors assertEventFailed to also check categories; fixes a pre-existing logic inversion bug (! was missing on re.MatchString); the new assertCategories helper does exact sorted match, but the guard that invokes it silently skips the check for empty expected categories, making it impossible to assert "zero categories" via the event stream.
internal/testsuite/app.go	Extends TestSpecsFromPattern to accept comma-separated glob patterns; adds "expected categories" substring to the Prometheus failure-detection heuristic. No deduplication when patterns produce overlapping paths.
docker-compose.yaml	Mounts the e2e executor config and passes --config to the executor, activating errorCategories rules in all docker-compose environments (not just CI e2e). Correct for the feature but adds a permanent dev-stack dependency on the e2e config file.
e2e/config/executor_config.yaml	New executor config file defining two errorCategories rules: OOMKilled condition → oom, exit codes {1,2,126,127} → user_error. Rules align correctly with the two test cases.
magefiles/ci.go	Extends the --tests flag to include testsuite/testcases/categorization/* alongside basic/*; relies on the new comma-separated glob splitting in TestSpecsFromPattern.
testsuite/testcases/categorization/oom.yaml	New e2e test: writes 256 MB to /dev/shm with a 25 Mi memory limit to trigger OOMKilled; asserts the failed event carries categories: ["oom"]. Well-formed for cgroup v2 CI environments.
testsuite/testcases/categorization/user_error.yaml	New e2e test: exit 1 triggers the user_error rule (exit codes In [1,2,126,127]); asserts the failed event carries categories: ["user_error"]. Straightforward and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Test YAML\nexpectedEvents.failed.categories] --> B{assertEventFailed}
    B --> C{reason != empty?}
    C -- yes --> D[compile regex\ncheck match]
    C -- no --> E{categories\nlen > 0?}
    D --> E
    E -- yes --> F[assertCategories\nexact sorted match\nslices.Equal]
    E -- no --> G[skip check\nreturn nil]
    F --> H{match?}
    H -- yes --> I[return nil ✓]
    H -- no --> J[return error\nexpected categories X but got Y]
    G --> I

Loading

Comments Outside Diff (3)

1. internal/testsuite/eventwatcher/validators.go, line 41-45 (link)

   Silent skip when no categories expected — cannot assert zero categories

   assertCategories is only invoked when the expected list is non-empty (line 41). When a test YAML omits categories: (or sets it to an empty list), the entire category check is skipped — the actual event's categories are never inspected.

   This means:

   • There is no way to write a test that asserts "this job should have received zero categories" via the event stream path.
   • If a misconfigured or accidentally-broad category rule is added to executor_config.yaml in the future, all existing basic test specs (which don't specify categories:) will continue to pass silently, regardless of what categories the job actually received.

   The two new test cases (oom.yaml and user_error.yaml) are safe because they do supply expected categories and the exact-match check will catch unexpected additions. But for other tests in basic/ the guard means category pollution is completely invisible.

   Consider inverting the guard so the check always runs — an empty expected list asserts an empty actual list:

   // Always check categories (empty expected ⟹ actual must also be empty)
   if err := assertCategories(expected.Failed.GetCategories(), actual.Failed.GetCategories()); err != nil {
       return err
   }

   If exact-match-on-empty is undesirable for backward compatibility with existing basic/ specs, you could instead document the limitation with a comment so future test authors know they cannot express "no categories" via this path.

2. internal/testsuite/app.go, line 90-103 (link)

   No deduplication when comma-separated glob patterns overlap

   TestSpecsFromPattern now splits the input on , and appends all results from each glob into allPaths without deduplication. If two patterns ever match the same file (e.g. testsuite/testcases/**/*,testsuite/testcases/categorization/*), the same test spec will be loaded and executed twice with no warning, potentially doubling the test run time or producing misleading duplicate results in the JUnit report.

   For the current patterns (basic/*,categorization/*) the directories are disjoint, so this is benign today — but adding a third, broader glob in the future could cause silent duplication.

   Consider deduplicating allPaths before returning:

   seen := make(map[string]struct{})
   for _, path := range allPaths {
       if _, ok := seen[path]; !ok {
           seen[path] = struct{}{}
           uniquePaths = append(uniquePaths, path)
       }
   }
   return TestSpecsFromFilePaths(uniquePaths)

3. docker-compose.yaml, line 133-141 (link)

   e2e executor config permanently changes the default developer stack

   Mounting ./e2e/config/executor_config.yaml and passing --config /config/executor_config.yaml to the executor affects every developer who runs docker-compose up, not just the CI e2e run. The errorCategories rules are now always active in local dev environments regardless of whether the developer is running the e2e suite.

   Two concrete consequences:

   1. Tight coupling to an e2e artefact — if e2e/config/executor_config.yaml is deleted or moved (e.g. during a reorganisation of the e2e/ directory), the docker-compose executor silently fails to start because the volume mount path no longer exists on the host.

   2. Undocumented dev-stack behaviour change — developers debugging failed jobs in the local stack will now see categories populated on failure events, which could be surprising and confusing if they are not aware of this PR. A comment in docker-compose.yaml near the new volume/command lines would help:

   # Mounts the e2e executor config to enable errorCategories rules
   # required by testsuite/testcases/categorization/* test cases.
   - "./e2e/config/executor_config.yaml:/config/executor_config.yaml"

_{Reviews (34): Last reviewed commit: "Add testsuite support for asserting erro..." | Re-trigger Greptile}

[dejanzele]

@greptile

[dejanzele]

@greptileai

[dejanzele]

@greptileai

[dejanzele]

@greptileai