add tas downstream tests - #1888
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new end-to-end test suite validating Topology‑Aware Scheduling (TAS) plus supporting test utilities to create/manage ChangesTAS end-to-end tests and supporting utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/e2e_tas_test.go (1)
770-770: 💤 Low valueRemove unused variable assignment.
The
topologyNameparameter is assigned to_but never used in the function body. This appears to be leftover code.Remove unused variable or document why it's kept
-func verifyWorkloadAdmittedWithTopology(ctx context.Context, namespace, jobUID string, expectedPodSets int, topologyName string) { +func verifyWorkloadAdmittedWithTopology(ctx context.Context, namespace, jobUID string, expectedPodSets int, _ string) { kueueClient := clients.UpstreamKueueClient - _ = topologyNameOr if you plan to use it later for additional assertions, document that with a TODO comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/e2e_tas_test.go` at line 770, The assignment "_ = topologyName" is a leftover unused variable; remove that line from the test (or if you intend to use topologyName later, replace the discard with a clear TODO comment referencing topologyName). Locate the occurrence of topologyName in the test function (search for the symbol topologyName in test/e2e/e2e_tas_test.go) and either delete the discard assignment or add a TODO explaining planned assertions that will use topologyName.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/e2e_tas_test.go`:
- Line 96: The test uses external images via
testutils.GetContainerImageForWorkloads() and must be marked to skip on
disconnected clusters: update the Ginkgo suite/Describe/It that contains the
Image: testutils.GetContainerImageForWorkloads() usages in e2e_tas_test.go to
include the disconnected skip label (e.g., add Label("Skipped:Disconnected") or
Label("Skipped:Disconnected") as an argument to the top-level
Describe/Context/It where the container image is set); apply this change for
every occurrence that references testutils.GetContainerImageForWorkloads() so
the tests are automatically skipped in disconnected environments.
- Around line 592-597: The helper labelWorkerNodesForDatacenterTopology
currently enforces at least 2 worker nodes which breaks Single Node OpenShift
(SNO) runs; update it to either detect SNO and call Skip() or relax the
requirement to work with available nodes. Specifically, modify
labelWorkerNodesForDatacenterTopology to query nodes (using
kubeClient.CoreV1().Nodes().List) and if len(nodes.Items) < 2 then call Ginkgo
Skip() with a clear message about SNO, or alternatively proceed using the single
worker node (adjusting downstream logic that assumes multiple nodes). Ensure the
change is made inside the labelWorkerNodesForDatacenterTopology function so
tests no longer fail on SNO.
---
Nitpick comments:
In `@test/e2e/e2e_tas_test.go`:
- Line 770: The assignment "_ = topologyName" is a leftover unused variable;
remove that line from the test (or if you intend to use topologyName later,
replace the discard with a clear TODO comment referencing topologyName). Locate
the occurrence of topologyName in the test function (search for the symbol
topologyName in test/e2e/e2e_tas_test.go) and either delete the discard
assignment or add a TODO explaining planned assertions that will use
topologyName.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 07576c03-fe31-4c5c-81fd-572c3fe07cf4
📒 Files selected for processing (2)
test/e2e/e2e_tas_test.gotest/e2e/testutils/utils.go
kannon92
force-pushed
the
add-tas-e2es
branch
4 times, most recently
from
May 15, 2026 21:10
8ba25a9 to
3c969eb
Compare
Contributor
Author
|
/retest |
kannon92
force-pushed
the
add-tas-e2es
branch
2 times, most recently
from
May 29, 2026 13:27
27ca0ad to
df27c5c
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
MaysaMacedo
reviewed
Jul 6, 2026
| localQueue *kueuev1beta2.LocalQueue | ||
| ) | ||
|
|
||
| BeforeEach(func(ctx context.Context) { |
Contributor
There was a problem hiding this comment.
This BeforeEach and JustAfterEach looks like the same from most of the other ginkgo When. Perhaps we could combine those When conditions that have same BeforeEach into one When to avoid code replication. WDYT?
Contributor
Author
There was a problem hiding this comment.
Good catch, done — combined the Job/JobSet/Pod/Deployment When blocks (which all shared the same hostname-topology BeforeEach/JustAfterEach) into a single When("Creating a workload requesting TAS with hostname topology", ...) block with separate Its. Pushed in 2ec0a0f.
Contributor
Author
|
/retest |
MaysaMacedo
reviewed
Jul 7, 2026
|
@kannon92: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kannon92, MaysaMacedo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Turns out the upstream tests hardcode kind nodes so it would be quite difficult to adapt the upstream tests.
So i went with a downstream.
Summary by CodeRabbit
Tests
Chores