Skip to content

test(helm): extend NodeSet tests for DRA-style claims and assertions#175

Open
giuliocalzo wants to merge 2 commits intoSlinkyProject:mainfrom
giuliocalzo:test/helm-nodeset-podspec-claims-unittest
Open

test(helm): extend NodeSet tests for DRA-style claims and assertions#175
giuliocalzo wants to merge 2 commits intoSlinkyProject:mainfrom
giuliocalzo:test/helm-nodeset-podspec-claims-unittest

Conversation

@giuliocalzo
Copy link
Copy Markdown
Contributor

Summary

Extends helm/slurm/tests/nodeset_test.yaml to cover Dynamic Resource Allocation-style pod configuration and to tighten a few assertions.

Changes

  • Resource claims: New test for podSpec.resourceClaims (asserted with contains) and podSpec.resources.claims (asserted with equal on spec.template.spec.resources).
  • Resources only: Renamed/split the prior limits/requests case into should set pod template resources without mixing in claim fields.
  • Affinity: Assert the full spec.template.spec.affinity object instead of a single nested leaf path.
  • Topology spread: Use contains on spec.template.spec.topologySpreadConstraints with the full constraint object.
  • Formatting: Blank lines between test cases for readability.

Verification

helm unittest helm/slurm -f 'tests/nodeset_test.yaml'

- Add pod template resourceClaims plus resources.claims coverage
- Split plain pod template resources into a dedicated test
- Assert full affinity block and topologySpreadConstraints via contains
- Add blank lines between test cases for readability
@vivian-hafener vivian-hafener self-requested a review May 6, 2026 17:47
Copy link
Copy Markdown
Contributor

@vivian-hafener vivian-hafener left a comment

Choose a reason for hiding this comment

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

I've requested changes in order to block the merge of this PR. We handle PR merges internally, instead of using GitHub, so we will handle the merge internally. Will close once merged internally per usual, just don't want to risk an accidental merge to main here as it is annoying to resolve.

Copy link
Copy Markdown
Contributor

@klairecodes klairecodes left a comment

Choose a reason for hiding this comment

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

Blocking merge.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants