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.

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

Running all the tests on two clusters and only full-rr on 3 clusters #769

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

jkremser
Copy link
Member

@jkremser jkremser commented Nov 23, 2021

Making TestFullRoundRobin dynamic in a way that it can run the assertions properly for any number of clusters (any ~ [2,8])

In makefile we should pass the build tag all xor rr_multicluster based on the number of clusters that currently runs. all will run the all the tests (same as before). rr_multicluster will run only the TestFullRoundRobin that depends on CLUSTERS_NUMBER env variable (which defaults to 2).

This way, we can also have a on dispatch GH action that can run the tests on-demand w/ the number of clusters as an input parameter of the action (not done in this PR).

Some data:
I've tried 4 attempts for these pipelines:

All 12 passed so I am assuming it's better than current master where it's pretty fragile.

Signed-off-by: Jirka Kremser jiri.kremser@gmail.com

Copy link
Collaborator

@kuritka kuritka left a comment

Choose a reason for hiding this comment

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

.github/workflows/terratest-more-clusters.yaml will be executed simultaneousy with terratest.yaml workflow ?

}

// helper function for de-duplicating elements in slice (in O(n))
func dedupe(s []string) (uniq []string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please rename the function to something more meaningful (e.g. in SQL wa have distinct keyword for that)

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to distinct

Comment on lines 28 to 31
if n < 2 || n > 8 {
t.Logf("Use value of n that represents the number of clusters from interval [2,8]")
t.FailNow()
} else {
t.Logf(fmt.Sprintf("Running TestFullRoundRobin for %d clusters", n))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid if else statements if you can, just invert condition and fail fast (e.g: crossplane doc).

Copy link
Member Author

Choose a reason for hiding this comment

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

right, the else branch is totally useless here :D (done)

@jkremser
Copy link
Member Author

.github/workflows/terratest-more-clusters.yaml will be executed simultaneousy with terratest.yaml workflow ?

y, that's how it's done in this PR. But I am open to also run it either on demand by dispatch action or nightly/"morningly" (cron), but as it was mentioned on the call, this way nobody will pay attention to the result and it may be hard to track the change that introduced some issue. Btw. it runs only TestFullRoundRobin nothing else.

.licignore Show resolved Hide resolved
terratest/test/k8gb_lifecycle_test.go Outdated Show resolved Hide resolved
terratest/test/k8gb_abstract_full_roundrobin_test.go Outdated Show resolved Hide resolved
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fixes!

@jkremser jkremser merged commit a2bec43 into k8gb-io:master Nov 26, 2021
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