[PS-2066] Warn when skip is unsupported for the runner#532
Draft
gchan wants to merge 1 commit into
Draft
Conversation
Test Engine honours skips by omitting tests from a node's task list, which only works for runners that split by example. File-splitting runners like Jest receive the whole file and run every test in it, so a test marked for skipping runs anyway and its failure fails the build, silently. Detect this via the filter_tests endpoint, which tags each returned file with a reason. Decode the previously discarded reason field and, for runners that cannot skip, warn loudly when any file carries the reason 'file contains 1 or more skipped tests'. Advisory only: nothing about what runs changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For runners that cannot skip tests (e.g. Jest), bktec now warns loudly when the suite has tests marked for skipping in files the runner will run, instead of failing the build silently.
Test Engine honours skips by omitting tests from a node's task list, which only works for runners that split by example. File-splitting runners like Jest receive the whole file and run every test in it, so a "skipped" test runs anyway and its failure fails the build with no indication that skip is unsupported — the behaviour a customer hit in PS-2061.
Detection uses the
filter_testsendpoint rather thantestPlan.SkippedTests. The server tags each returned file with areason("slow file"or"file contains 1 or more skipped tests"); skipped files are always returned, slow files only whensplit_by_exampleis true. The client previously decoded onlypathand discardedreason. This change decodesreasonand, for non-skip runners, warns when any file carries the skip reason.Decisions Made
filter_tests, not the Quarantine REST API.GET .../tests/skippedreturns example-level data but is suite-wide, so it over-reports tests from files this build doesn't run.filter_testsis build-scoped (filters against the POSTed files).Advisory only. The warning never mutes, never alters the task list, and never changes the exit code. Errors from
filter_tests(including the endpoint's retryable "still filtering" responses) are logged to debug and ignored, since this is off the critical path.File-level, not test-level.
filter_testsreturns files, not examples, so the warning names the affected files but not the individual skipped tests. This is the same client-side limitation that made the PS-2065 mute-fallback unworkable.Context
Linear: PS-2066, related to PS-2061. Alternative to PS-2065 (#531, closed).
Verification
go test ./internal/command/ ./internal/api/— backed by specs. New tests cover: a skip-reason file produces a warning (and a slow-reason file in the same response does not), no warning when only slow files are returned, and skip-capable runners (RSpec) skip the check entirely (nofilter_testscall). Server contract (reasonvalues, skip-vs-slow gating) fact-checked against thebuildkiteRails app. AI assisted.Rollback
Yes. Additive, advisory-only change behind a runner-capability check; reverting the commit removes the warning with no behavioural change.