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

Jump to bottom

Fix ENT Tests Now that They Are Running Again 🏃 #3077

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Fix ENT Tests Now that They Are Running Again 🏃 #3077

merged 1 commit into from
Oct 13, 2023

Conversation

DanStough
Copy link
Contributor

@DanStough DanStough commented Oct 12, 2023 •
edited
Loading

ENT tests were just turned back on in CI. This revealed some failing test cases. This PR attempts to get everything in working order.

@DanStough DanStough added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport signals that a PR will not contain a backport label do-not-merge labels Oct 12, 2023
@DanStough DanStough changed the title DNM see if ENT tests are running Fix ENT Tests Now that They Are Running Again 🏃 Oct 13, 2023
@DanStough DanStough marked this pull request as draft October 13, 2023 04:34
@DanStough DanStough marked this pull request as ready for review October 13, 2023 14:44
DanStough
DanStough commented Oct 13, 2023
View reviewed changes
control-plane/config-entries/controllersv2/meshconfig_controller_test.go Outdated
Comment on lines 148 to 152
require.Eventually(t, func() bool {
_, _, err := testClient.APIClient.Partitions().Read(context.Background(), constants.DefaultConsulPartition, nil)
if err != nil {
return false
}
return true
}, 5*time.Second, 500*time.Millisecond)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, this moves to Consul's sdk, but it needs to be done in a way that works across V1 and V2 that will require some more thought, more than cleaning up test failure on main.

@DanStough
Copy link
Contributor Author

There are still flakes in these tests, but it would seem not enough that would prevent them from passing with successive gotestsum runs.

zalimeni
zalimeni approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@zalimeni zalimeni left a comment

Choose a reason for hiding this comment

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

Thank you for fixing these!

control-plane/connect-inject/controllers/peering/peering_acceptor_controller_test.go Show resolved Hide resolved
control-plane/connect-inject/controllers/pod/pod_controller_test.go Show resolved Hide resolved
control-plane/subcommand/server-acl-init/command_ent_test.go Outdated Show resolved Hide resolved
control-plane/config-entries/controllersv2/meshconfig_controller_test.go Show resolved Hide resolved
control-plane/connect-inject/controllers/pod/pod_controller_ent_test.go Outdated Show resolved Hide resolved
@DanStough
Copy link
Contributor Author

@zalimeni re: confused emoji: gotestsum reruns any failed tests up to 3 times, so it can flake occasionally but not block CI. I wouldn't say to rely on it, but trying to balance out the investment in fixing this before a release.

@DanStough DanStough force-pushed the dans/fix-enterprise-flakes branch 2 times, most recently from 6d1561c to 5f36df8 Compare October 13, 2023 16:31
@DanStough DanStough enabled auto-merge (squash) October 13, 2023 16:32
@zalimeni
Copy link
Member

@zalimeni re: confused emoji:

Thank you for the context! I was actually just looking for the closest thing to a sad face, bc there are still flakes despite all your efforts here. I think that approach is super reasonable.

thisisnotashwin
thisisnotashwin approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

:chef-kiss:

@zalimeni
Copy link
Member

zalimeni commented Oct 13, 2023 •
edited
Loading

Looking at some of these failures to lend a hand since they're still looking weird.

=== FAIL: subcommand/fetch-server-region TestRun_FlagValidation/missing_node_name (0.00s)
    command_test.go:46: 
        	Error Trace:	/home/runner/actions-runner/_work/consul-k8s-workflows/consul-k8s-workflows/control-plane/subcommand/fetch-server-region/command_test.go:46
        	Error:      	"-output-file is required\n-output-file is required\n" does not contain "-node-name is required"
        	Test:       	TestRun_FlagValidation/missing_node_name

Just noting here that this looks like a race w/ the synced buffer underlying MockUI (? why is this necessary?)... Going to leave that be until we see more of them and focus on the pod controller tests.

My theory is that the reruns of the other failed tests are causing the sync buffer to get out of whack due to reuse. No proof, but hard to otherwise explain how this test is suddenly now impossibly failing.

@nathancoleman nathancoleman mentioned this pull request Oct 13, 2023
Merged
2 tasks
@zalimeni
Copy link
Member

Tests are passing now, with @DanStough's fixes and nothing disabled.

I believe inconsistent failures came from main and release/1.17.x on consul sharing a VERSION file, leading them to race each other to overwrite hashicorppreview/consul-1.17-dev. That plus some missing backports earlier today would have been enough to flip-flop expected behavior without these tests changing.

No-op amended the commit to get a fresh SHA so none of the GHA results were old.

@zalimeni zalimeni enabled auto-merge (squash) October 13, 2023 22:48
@zalimeni zalimeni merged commit 0b6d4eb into main Oct 13, 2023
24 of 48 checks passed
@zalimeni zalimeni deleted the dans/fix-enterprise-flakes branch October 13, 2023 22:55
nathancoleman pushed a commit that referenced this pull request Oct 16, 2023
@DanStough @nathancoleman
Fix ENT Tests Now that They Are Running Again 🏃 (#3077)
13bb996 
test: fix tests that are failing on main
@nathancoleman nathancoleman mentioned this pull request Oct 16, 2023
Merged
nathancoleman pushed a commit that referenced this pull request Oct 16, 2023
@DanStough @nathancoleman
Fix ENT Tests Now that They Are Running Again 🏃 (#3077)
fd87182 
test: fix tests that are failing on main
@nathancoleman nathancoleman mentioned this pull request Oct 16, 2023
Merged
nathancoleman added a commit that referenced this pull request Oct 16, 2023
@nathancoleman @DanStough
Backport of Fix ENT Tests Now that They Are Running Again 🏃 into rele…
24e10f1 
…ase/1.2.x (#3082)

Fix ENT Tests Now that They Are Running Again 🏃  (#3077)

test: fix tests that are failing on main

Co-authored-by: Dan Stough <dan.stough@hashicorp.com>
zalimeni pushed a commit that referenced this pull request Oct 17, 2023
@nathancoleman @DanStough
Backport of Fix ENT Tests Now that They Are Running Again 🏃 into rele…
95b51bd 
…ase/1.1.x (#3084)

Fix ENT Tests Now that They Are Running Again 🏃  (#3077)

test: fix tests that are failing on main

Co-authored-by: Dan Stough <dan.stough@hashicorp.com>
zalimeni pushed a commit that referenced this pull request Oct 17, 2023
@DanStough @zalimeni
Fix ENT Tests Now that They Are Running Again 🏃 (#3077)
d4fb33f 
test: fix tests that are failing on main
@zalimeni zalimeni mentioned this pull request Oct 17, 2023
Merged
zalimeni added a commit that referenced this pull request Oct 17, 2023
@zalimeni @DanStough
Backport of Fix ENT Tests Now that They Are Running Again 🏃 into rele…
098b558 
…ase/1.0.x (#3090)

Fix ENT Tests Now that They Are Running Again 🏃  (#3077)

test: fix tests that are failing on main

Co-authored-by: Dan Stough <dan.stough@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zalimeni zalimeni zalimeni approved these changes

@nathancoleman nathancoleman nathancoleman left review comments

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

Assignees
No one assigned
Labels
pr/no-backport signals that a PR will not contain a backport label pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DanStough @zalimeni @nathancoleman @thisisnotashwin