Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a new automated review policy file Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/cli/fauxtests.go`:
- Around line 40-47: The test in g.It ("should be able to contact the foo
endpoint") calls http.Get against a hardcoded 127.0.0.1:8080 which is never
started and breaks IPv6 policy; replace the external call with an in-test HTTP
fixture: create an httptest.NewServer with a handler for "/foo" that returns
http.StatusOK, use server.URL (not a hardcoded host/port) for the request, call
http.Get(server.URL + "/foo"), and defer server.Close(); update the assertions
to use the response from that server instead of the unreachable address.
- Around line 34-37: This test must not rely on cluster-provided BuildConfigs;
before calling oc.Run("get").Args("buildconfigs", "-n", "default").Output()
inside the g.It block, either create a minimal BuildConfig fixture in the test
namespace using oc.Run("create"/"apply") (and ensure cleanup after the test) or
detect that the server supports the buildconfigs API and call framework.Skipf
(or equivalent) to skip the test on unsupported platforms; update the g.It
anonymous test to perform the create-or-skip logic using oc.Run and the same
namespace used in the test rather than assuming objects exist in "default".
- Around line 50-55: The test "should have at least three nodes" hardcodes a
3-node expectation and fails on single-node clusters; change it to first detect
cluster topology or single-replica mode and only assert >=3 nodes when
appropriate. Use the existing oc helper (oc.Run("get")...) to fetch the
Infrastructure resource (e.g.,
oc.Run("get").Args("infrastructures.config.openshift.io", "cluster", "-o",
"jsonpath={.status.controlPlaneTopology}") or similar) or check node count and
skip/adjust the assertion: if topology indicates SingleReplica or node count <3,
either skip the test or assert the expected single-node behavior, otherwise keep
the existing len(lines) >= 3 check; update the g.It block and related variables
(out, lines) accordingly so the test gates on cluster topology instead of
hardcoding 3 nodes.
- Around line 17-20: The test helper randomString() produces non-deterministic
It(...) titles and violates the deterministic-name check; remove the
randomString() function and replace any uses in test descriptions (the It(...)
calls that reference randomString()) with stable, deterministic literal strings
(e.g., a fixed descriptive name) so test names remain constant across runs;
update or simplify any helper references to use the new fixed names and delete
the randomString() function (and its import usage) from
test/extended/cli/fauxtests.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d423c97e-9cee-46b4-bed0-b5604eb28272
📒 Files selected for processing (2)
.coderabbit.yamltest/extended/cli/fauxtests.go
test/extended/cli/fauxtests.go
Outdated
| g.It(fmt.Sprintf("should list BuildConfigs in default namespace %s", randomString()), func() { | ||
| out, err := oc.Run("get").Args("buildconfigs", "-n", "default").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(out).NotTo(o.BeEmpty()) |
There was a problem hiding this comment.
Don’t depend on pre-existing BuildConfigs in default.
This spec assumes two things that are not stable: that the cluster exposes the OpenShift buildconfigs API, and that default already contains at least one object. Both assumptions are false on some supported jobs, including MicroShift. Create the fixture in the test namespace or gate/skip the test on unsupported platforms instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/cli/fauxtests.go` around lines 34 - 37, This test must not rely
on cluster-provided BuildConfigs; before calling
oc.Run("get").Args("buildconfigs", "-n", "default").Output() inside the g.It
block, either create a minimal BuildConfig fixture in the test namespace using
oc.Run("create"/"apply") (and ensure cleanup after the test) or detect that the
server supports the buildconfigs API and call framework.Skipf (or equivalent) to
skip the test on unsupported platforms; update the g.It anonymous test to
perform the create-or-skip logic using oc.Run and the same namespace used in the
test rather than assuming objects exist in "default".
| g.It("should be able to contact the foo endpoint", func() { | ||
| host := "127.0.0.1" | ||
| port := 8080 | ||
| url := fmt.Sprintf("http://%s:%d/foo", host, port) | ||
| resp, err := http.Get(url) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer resp.Body.Close() | ||
| o.Expect(resp.StatusCode).To(o.Equal(http.StatusOK)) |
There was a problem hiding this comment.
This test calls an endpoint that the spec never provisions.
Nothing in this file starts a server on 127.0.0.1:8080, so this will normally fail with connection refused. It also hardcodes IPv4 localhost, which conflicts with the IPv6-compatibility policy added in this PR. If the goal is a local-process check, use an in-test fixture; if the goal is cluster networking, create and probe a cluster-backed endpoint instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/cli/fauxtests.go` around lines 40 - 47, The test in g.It
("should be able to contact the foo endpoint") calls http.Get against a
hardcoded 127.0.0.1:8080 which is never started and breaks IPv6 policy; replace
the external call with an in-test HTTP fixture: create an httptest.NewServer
with a handler for "/foo" that returns http.StatusOK, use server.URL (not a
hardcoded host/port) for the request, call http.Get(server.URL + "/foo"), and
defer server.Close(); update the assertions to use the response from that server
instead of the unreachable address.
| g.It("should have at least three nodes", func() { | ||
| out, err := oc.Run("get").Args("nodes", "-o", "name").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| lines := strings.Split(strings.TrimSpace(out), "\n") | ||
| o.Expect(len(lines)).To(o.BeNumerically(">=", 3), "expected at least 3 nodes, got %d", len(lines)) | ||
| }) |
There was a problem hiding this comment.
Avoid hardcoding a 3-node requirement.
This spec bakes in a multi-node cluster assumption and will fail on Single Node OpenShift and MicroShift, both called out in the new pre-merge checks. Gate it on topology or change the assertion to something that is valid regardless of node count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/cli/fauxtests.go` around lines 50 - 55, The test "should have
at least three nodes" hardcodes a 3-node expectation and fails on single-node
clusters; change it to first detect cluster topology or single-replica mode and
only assert >=3 nodes when appropriate. Use the existing oc helper
(oc.Run("get")...) to fetch the Infrastructure resource (e.g.,
oc.Run("get").Args("infrastructures.config.openshift.io", "cluster", "-o",
"jsonpath={.status.controlPlaneTopology}") or similar) or check node count and
skip/adjust the assertion: if topology indicates SingleReplica or node count <3,
either skip the test or assert the expected single-node behavior, otherwise keep
the existing len(lines) >= 3 check; update the g.It block and related variables
(out, lines) accordingly so the test gates on cluster topology instead of
hardcoding 3 nodes.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin 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 |
openshift-ci
bot
added
the
approved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
test/extended/cli/fauxtests.go (4)
17-28:⚠️ Potential issue | 🟠 MajorRemove randomized spec naming to keep test IDs deterministic.
Line 28 builds the test name with
randomString(), making the spec name different on every run.Proposed fix
-import ( - "crypto/rand" - "encoding/hex" - "fmt" +import ( + "fmt" "net/http" "strings" @@ -func randomString() string { - b := make([]byte, 8) - _, _ = rand.Read(b) - return hex.EncodeToString(b) -} - var _ = g.Describe("[sig-cli] oc fauxtests", func() { @@ - g.It(fmt.Sprintf("should pass the %s faux test", randomString()), func() { + g.It("should report client version", func() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/cli/fauxtests.go` around lines 17 - 28, The test spec name is non-deterministic because g.It uses randomString() to build the description; remove the randomized suffix and make the spec name deterministic by replacing the fmt.Sprintf("should pass the %s faux test", randomString()) call with a constant or stable string (e.g., "should pass the faux test"); also remove or keep randomString() only if used elsewhere, and update references in this file (randomString, g.It) so test IDs remain stable across runs.
40-47:⚠️ Potential issue | 🔴 CriticalReplace hardcoded localhost call with an in-test HTTP fixture.
Lines 41-44 call
127.0.0.1:8080without creating a server, so this spec is expected to fail in CI.Proposed fix
import ( "fmt" "net/http" + "net/http/httptest" "strings" @@ g.It("should be able to contact the foo endpoint", func() { - host := "127.0.0.1" - port := 8080 - url := fmt.Sprintf("http://%s:%d/foo", host, port) - resp, err := http.Get(url) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/foo" { + w.WriteHeader(http.StatusNotFound) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + resp, err := http.Get(srv.URL + "/foo") o.Expect(err).NotTo(o.HaveOccurred()) defer resp.Body.Close() o.Expect(resp.StatusCode).To(o.Equal(http.StatusOK)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/cli/fauxtests.go` around lines 40 - 47, The test currently calls a hardcoded host/port (127.0.0.1:8080) without starting a server; replace that with an in-test HTTP fixture by creating an httptest.Server that handles the "/foo" path and returns http.StatusOK, use server.URL+"/foo" as the request URL, call http.Get against that URL (keeping resp and err checks), and ensure you defer server.Close() and resp.Body.Close(); update imports to include net/http/httptest as needed.
34-37:⚠️ Potential issue | 🟠 MajorDon’t rely on pre-existing
BuildConfigobjects indefault.Lines 35-37 assume both API availability and existing resources in
default, which is not stable across supported jobs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/cli/fauxtests.go` around lines 34 - 37, The test block g.It currently assumes BuildConfig API and resources exist by calling oc.Run("get").Args("buildconfigs", "-n", "default").Output(); instead, make the test create its own BuildConfig in an isolated namespace (or create a temporary namespace) before calling oc.Run("get") and then delete it as cleanup. Concretely, update the g.It test to: create/ensure a namespace, use oc.Run("create").Args(...) or oc.Run("apply").Args(...) to create a minimal BuildConfig resource, call oc.Run("get").Args("buildconfigs", "-n", <testNS>).Output() to assert presence, and remove the resource/namespace in AfterEach to avoid relying on pre-existing state.
50-55:⚠️ Potential issue | 🟠 MajorAvoid hardcoding a 3-node requirement.
Lines 54-55 enforce
>= 3nodes unconditionally, which fails on valid single-node topologies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/cli/fauxtests.go` around lines 50 - 55, The test "should have at least three nodes" hardcodes a minimum of 3 and should be made configurable; modify the test in the It block (the function containing oc.Run("get").Args("nodes", "-o", "name").Output(), variables out/err, lines and the o.Expect(len(lines)).To(...)) to compute the required minimum from a configurable source (for example read an env var like TEST_MIN_NODES with a sensible default of 1) and parse it to an int (handle parse errors by failing the test), then replace the literal 3 with that parsed minNodes value in the o.Expect assertion so single-node topologies pass by default but tests can require more nodes via configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/cli/fauxtests.go`:
- Around line 17-28: The test spec name is non-deterministic because g.It uses
randomString() to build the description; remove the randomized suffix and make
the spec name deterministic by replacing the fmt.Sprintf("should pass the %s
faux test", randomString()) call with a constant or stable string (e.g., "should
pass the faux test"); also remove or keep randomString() only if used elsewhere,
and update references in this file (randomString, g.It) so test IDs remain
stable across runs.
- Around line 40-47: The test currently calls a hardcoded host/port
(127.0.0.1:8080) without starting a server; replace that with an in-test HTTP
fixture by creating an httptest.Server that handles the "/foo" path and returns
http.StatusOK, use server.URL+"/foo" as the request URL, call http.Get against
that URL (keeping resp and err checks), and ensure you defer server.Close() and
resp.Body.Close(); update imports to include net/http/httptest as needed.
- Around line 34-37: The test block g.It currently assumes BuildConfig API and
resources exist by calling oc.Run("get").Args("buildconfigs", "-n",
"default").Output(); instead, make the test create its own BuildConfig in an
isolated namespace (or create a temporary namespace) before calling
oc.Run("get") and then delete it as cleanup. Concretely, update the g.It test
to: create/ensure a namespace, use oc.Run("create").Args(...) or
oc.Run("apply").Args(...) to create a minimal BuildConfig resource, call
oc.Run("get").Args("buildconfigs", "-n", <testNS>).Output() to assert presence,
and remove the resource/namespace in AfterEach to avoid relying on pre-existing
state.
- Around line 50-55: The test "should have at least three nodes" hardcodes a
minimum of 3 and should be made configurable; modify the test in the It block
(the function containing oc.Run("get").Args("nodes", "-o", "name").Output(),
variables out/err, lines and the o.Expect(len(lines)).To(...)) to compute the
required minimum from a configurable source (for example read an env var like
TEST_MIN_NODES with a sensible default of 1) and parse it to an int (handle
parse errors by failing the test), then replace the literal 3 with that parsed
minNodes value in the o.Expect assertion so single-node topologies pass by
default but tests can require more nodes via configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c61a4c7-fabb-427a-a363-84323443c18d
📒 Files selected for processing (1)
test/extended/cli/fauxtests.go
|
/close |
|
@dgoodwin: Closed this PR. DetailsIn response to this:
Instructions 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. |
|
@dgoodwin: The following test 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. |
2 participants
This should flag heavily.
Summary by CodeRabbit
Chores
Tests