-
Notifications
You must be signed in to change notification settings - Fork 813
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.
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
Add e2e test for Extensions #2947
Conversation
Build Failed 😱 Build Id: 7f2d243c-5364-40e7-a80f-200f6b648751 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: de8179f4-e552-4fe6-bff9-01b5c8ccde2b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
d87b344
to
2d38ee8
Compare
Build Succeeded 👏 Build Id: 8e65e5ac-3dc5-4339-a362-f6a28bc89ee1 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 3016787c-4f6f-459c-a4a5-38c94575da33 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
6d62674
to
f2e91f4
Compare
Build Failed 😱 Build Id: fcab7a59-4821-47cd-b79d-83f7eba4a238 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: b976e379-41bf-43d5-ba66-e7d5621294fd The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Failed 😱 Build Id: 1274e469-4b66-45bd-9e34-f72e3770ef64 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 3437bcd4-2222-4bd3-a54e-afbf146bd2ca The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
0837dc4
to
883ee6f
Compare
Build Succeeded 👏 Build Id: 43a885e2-293a-4b9a-9999-33a6a9c0edbd The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: feb60f68-9643-48ad-b6cb-b2c81d8e57f3 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -354,6 +355,24 @@ else | |||
endif | |||
echo "Finishing e2e controller failure test!" | |||
|
|||
test-e2e-ha-extensions: FEATURE_GATES ?= $(ALPHA_FEATURE_GATES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: I would like us to look at reducing redundancy here since we're repeating ourselves a lot. It's not clear to me that the original split couldn't be done a different way - it might be nice to try folding all of this back into the main e2e package. But certainly this is fine for this PR.
ctx := context.Background() | ||
|
||
if !runtime.FeatureEnabled(runtime.FeatureSplitControllerAndExtensions) { | ||
logger.Infof("Skip test. SplitControllerAndExtensions feature is not enabled: flag is %v", runtime.FeatureEnabled(runtime.FeatureSplitControllerAndExtensions)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use t.Skip() here: https://pkg.go.dev/testing#hdr-Skipping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
logger.WithField("gsKey", readyGs.ObjectMeta.Name).Info("GameServer Ready") | ||
|
||
gsClient := framework.AgonesClient.AgonesV1().GameServers(defaultNs) | ||
defer gsClient.Delete(ctx, readyGs.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint: errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's literally the last thing we do it's a little funny to defer it. Plus you can just collapse the helper variable here, and re-lintcheck it. So:
assert.NoError(t, framework.AgonesClient.AgonesV1().GameServers(defaultNs).Delete(
ctx, readyGs.ObjectMeta.Name, metav1.DeleteOptions{})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup it is 😅 combined the lines.
// faking a extensions pod crash. | ||
func deleteAgonesExtensionsPods(ctx context.Context) error { | ||
list, err := getAgoneseExtensionsPods(ctx) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and below, I'd be inclined to throw in a t *testing.T
parameter and just use assert.NoError(t, err)
. Keeps it more concise and better tracks the line number of failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! As for the method below waitForAgonesExtensionsRunning
, I don't think it makes sense to add t *testing.T
as it won't be able to use it since it still needs to return the bool.
|
||
list, err := getAgoneseExtensionsPods(ctx) | ||
logger.Infof("Length of pod list is %v", len(list.Items)) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use assert.NoError
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
if err != nil { | ||
t.Fatalf("Could not get list of Extension pods: %v", err) | ||
} | ||
if len(list.Items) <= 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use assert.Greater
. The message is nice, though, so you can keep:
assert.Greater(list.Items, 1, "Cluster has zero or one extension pods")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thats neat, I did not know this was an option.
logger.Info("Creating default game server") | ||
gs := framework.DefaultGameServer(defaultNs) | ||
readyGs, err := framework.CreateGameServerAndWaitUntilReady(t, defaultNs, gs) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NoError()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
Build Failed 😱 Build Id: bcd43570-9a7a-4eca-9d62-aa0244e2e301 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: a8c90adb-ad7f-4479-915b-0bad7ee5f014 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Started e2e test for extensions
Build Succeeded 👏 Build Id: 7793eb64-aef5-43a0-a2d9-2592cb4bdf62 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiayi, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Adds an E2E test for testing high availability extensions.
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Part of HA Agones. Adds an e2e test that will test high availability by deleting one of the extensions pod and creating a gameserver.
Which issue(s) this PR fixes:
Work on #2797
Special notes for your reviewer: