game-ci / game-ci/unity-test-runner

Proposal: stop reporting skipped/ignored tests as a warning

Open
#328 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
265
Forks
149
Avg merge
3h 3m
Merged PRs (30d)
2

Description

### Summary

A test run that contains skipped tests but no failures is currently reported with a ⚠️ warning mark, even when every skip is a deliberate `[Ignore]` / `[Explicit]`. I'd like to change that so intentional skips don't look like something went wrong, but it changes visible output, so I wanted to ask before opening a PR whether v5 is the right place for it.

### Current behavior

The mark for a run is decided by `RunMeta.mark`:

```ts
get mark() {
if (this.failed > 0) return '❌️';
else if (this.skipped === 0) return '✅';
return '⚠️';
}
```

and per test by `TestMeta.mark`, which returns `⚠️` for `isSkipped()`.

So `100 passed, 3 ignored, 0 failed` renders as:

```
⚠️ editmode-results.xml - 100/103, skipped: 3 - Passed in 12.345s
```

### Why this is a problem

`[Ignore("...")]` and `[Explicit]` are *decisions*, not incidents. A suite that intentionally excludes a few tests is a healthy suite, and marking it ⚠️ trains people to ignore the warning mark — which then hides the cases that genuinely deserve attention.

### Proposed change

Don't let a skipped test downgrade the mark:

- `RunMeta.mark`: `failed > 0 → ❌️`, otherwise `✅`. The skipped count stays visible in the summary text (`, skipped: N`), so nothing is hidden — it just stops being styled as a warning.
- `TestMeta.mark`: use a neutral mark for skipped tests (e.g. `⏭️` or `➖`) instead of `⚠️`.

### Optional variant

If we'd rather keep a warning for *some* skips, we can gate it on `label`: `Ignored` / `Explicit` are intentional and get the neutral mark.

NUnit already tells us why a test was skipped. Skipped test cases carry a `label` attribute and a `_SKIPREASON` property:

```xml




```

Today neither `label` nor `` is read by the results parser at all — only the `result` attribute is. Making the parser carry them through is a prerequisite for the optional variant below, and useful for several other improvements to this same code path.

I'm happy either way — I'd lean toward the simpler version above, since in practice a `result="Skipped"` test case always carries one of those labels.

### Compatibility

This changes the rendered GitHub Check output (title, summary, details). It does **not** change the job's pass/fail result, but anyone who eyeballs — or scrapes — the check output will see different marks after this, so it feels like a breaking change to me rather than a patch.
For that reason, I haven't reported this until now, but I would like you to consider it when upgrading to v5.

### Related

Other issues exist within the same "result analysis" processing path. I plan to submit pull requests (PRs) for those as well once we agree on the approach.

- `Inconclusive` results are counted as skipped and never fail the job — `RunMeta.addTest` classifies anything that isn't `Passed` or `Failed` as skipped, and the `inconclusive` attribute on `` is never read.
- Parameterized test cases are hoisted out of their fixture in the details view — they end up as a sibling top-level suite named after the method, so the fixture row looks empty, and its counts exclude them.
- Flaky tests aren't surfaced at all — the `retryIteration` property in the result XML tells us a test only passed on a retry, which is exactly the case that deserves the ⚠️ we're removing from intentional skips.

### Feedback Requested

- Should I submit a PR to fix the `skip` behavior?
- Regarding the implementation: should it be simple, or should it branch based on the specific reason for the skip?
- For the related issues, should I submit direct PRs for each, or discuss them individually in separate issues?

Contributor guide

Open the contributing guide

Research direction

Start at RunMeta.mark and TestMeta.mark, then trace the results parser that currently reads only the result attribute. Confirm whether the agreed scope is the simple neutral treatment or label-aware handling. Done means skipped counts remain visible while intentional skips no longer use warning marks, with the GitHub Check output updated accordingly.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, typescript
Domain
ci-cd, testing-qa
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.