Skip to content

Add TestAdminActionMFA to tests to skip for flaky test detector#35882

Merged
Joerger merged 2 commits intomasterfrom
joerger/skip-tctl-admin-action-tests-flaky-check
Dec 19, 2023
Merged

Add TestAdminActionMFA to tests to skip for flaky test detector#35882
Joerger merged 2 commits intomasterfrom
joerger/skip-tctl-admin-action-tests-flaky-check

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Dec 19, 2023

Unblocks several open PRs since TestAdminActionMFA can never pass the flaky test detector, with an average runtime of 8 seconds.

I thought #35806 would allow me to merge my PRs one by one without tripping the flaky test detector, but it turns out I misunderstood the logic in difftest.

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Dec 19, 2023
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Dec 19, 2023

I know you've been stuck on this a bit, so I don't want to slow you down, but this should be the last resort if there are no other options. We shouldn't be writing new unit tests that take 8 seconds to complete. Is there anything we can do to speed this test up?

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Dec 19, 2023

I know you've been stuck on this a bit, so I don't want to slow you down, but this should be the last resort if there are no other options. We shouldn't be writing new unit tests that take 8 seconds to complete. Is there anything we can do to speed this test up?

It's because this is an e2e test that starts an actual Teleport cluster, from what I can tell e2e tests like this almost always take >5 seconds. Splitting them into their own tests instead of subtests would avoid tripping the flaky test detector at the cost of actually spinning up more test clusters and taking longer overall.

@rosstimothy and I discussed this and decided this was a good feature to e2e test to avoid extra manual tests in the test plan. The feature is also unit tested so it is somewhat redundant. Curious what you think we should do, I'd be happy to return to this after the feature is complete.

@Joerger Joerger disabled auto-merge December 19, 2023 18:25
@rosstimothy
Copy link
Copy Markdown
Contributor

I think an end to end test is still desirable to exercise the full path. Would each individual test be fast enough to satisfy the flaky test detector if each resource was its own distinct test instead of a single test with many subtests? For example, what if we had TestAdminActionMFAUser, TestAdminActionMFARole, etc. instead of TestAdminActionMFA/User, TestAdminActionMFA/Role, etc.

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Dec 19, 2023

I think an end to end test is still desirable to exercise the full path. Would each individual test be fast enough to satisfy the flaky test detector if each resource was its own distinct test instead of a single test with many subtests? For example, what if we had TestAdminActionMFAUser, TestAdminActionMFARole, etc. instead of TestAdminActionMFA/User, TestAdminActionMFA/Role, etc.

Yes, but then we would have 10+ tests that take 5-6 seconds each instead of 1 test that takes 8-10 seconds. I think I've done this in the past to avoid being slowed down by the flaky test detector, but it seems very counterproductive.

@Joerger Joerger added this pull request to the merge queue Dec 19, 2023
Merged via the queue into master with commit 830c5ac Dec 19, 2023
@Joerger Joerger deleted the joerger/skip-tctl-admin-action-tests-flaky-check branch December 19, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants