Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix install-cni v2 template test cases broken by #297 #299

Merged
merged 1 commit into from
Apr 11, 2024
+14 −0
Merged

Fix install-cni v2 template test cases broken by #297 #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions scripts/testcase/testcase-watchdog-cilium-faststart-unhealthy-v2.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ function before_test() {
}
export -f curl

# shellcheck disable=SC2317
function sleep() {
echo "[MOCK called] sleep $*"
echo "[MOCK] this test expects a delay during fast start."
Copy link
Collaborator

Choose a reason for hiding this comment

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

how did the test passed before then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because #269 was a merge, without rebasing to pick up #297 changes...

Copy link
Member

@marqc marqc Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I suspect they just took much longer to pass, as it was actually sleeping/waiting for 60seconds.

This mock function does not verify anything.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. You're right. It should have failed on https://github.com/GoogleCloudPlatform/netd/blob/master/scripts/test-install-cni.sh#L48

Copy link
Member

Choose a reason for hiding this comment

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

@aojea I see now. There were 2 PRs merged and each of them was working at the time they were sent and verified.
#269 and #297

So only after they were merged together into the master, the tests started failing. So probably we should have rebase #269 before merging on top of the master branch as this PR was waiting a few weeks before getting merged. The test logic works ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC github actions does not rebase, so it test against the state of the repo that was at that point

}
export -f sleep

}

function verify() {
Expand Down
7 changes: 7 additions & 0 deletions scripts/testcase/testcase-watchdog-cilium-faststart-v2.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ function before_test() {
}
export -f curl

# shellcheck disable=SC2317
function sleep() {
echo "[MOCK called] sleep $*"
echo "[MOCK] this test expects a delay during fast start."
}
export -f sleep

}

function verify() {
Expand Down
Loading