fix: copy config.group onto source and singular tests - #16265
Closed
larspettermadsstuen wants to merge 2 commits into
Closed
larspettermadsstuen wants to merge 2 commits into
larspettermadsstuen wants to merge 2 commits into
Conversation
Unattached tests were dropping config.group from the top-level group field. Fall back to the explicit config when there is no attached node. Co-authored-by: Cursor <cursoragent@cursor.com>
aiguofer
added
the
ci:approve-public-fork-ci
Contributor
|
Thank you very much for your contribution! your fix has been merged |
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
2 participants
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.
Resolves #15930
@waterWang already fixed this in #15940. That PR is still open, but it looks stuck: CLA unsigned, no changelog, and it's gone dirty against main. I asked there whether they were going to finish it; no movement, and other people still want the fix. Opening a fresh PR so this can actually land.
The code change is theirs — I reapplied it on current main and added the changelog. Credit for the diagnosis and the patch goes to them.
Problem
On Fusion, source tests and singular tests never get a top-level
group, even when you setconfig.group. Core 1.x copies that config ontogroup. Selectors likegroup:…,state:modified, and group notifications all look at the top-level field, so those tests quietly drop out of group-based workflows.Generic tests on models are already fine — they inherit the parent model's group, and we should leave that alone (including the case where the parent has no group).
Solution
If the test has no
attached_node, useconfig.group. If it does (model/seed/snapshot), keep inheriting the parent.#15940 doesn't include a repro. The parse setup and how to check
config.groupvsgroupare in #15930 — I ran that on a local build of this branch and the three bug rows now getgroup=test_group.No new unit test — the check that matters is parse + the manifest field.
Checklist
Made with Cursor