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

[TT-13048] Flaky test due to using httpbin org, improvements #6504

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Sep 11, 2024

Using httpbin.org is coupling our CI to their SLA, causing failing tests.

Test only change for:

  • testing httpbin exposed on 3123
  • fixes tests/regression flakyness due to httpbin.org usage
  • t.Cleanup cleanup instead of defering server close in area (limited scope)

https://tyktech.atlassian.net/browse/TT-13048

@titpetric titpetric requested a review from a team as a code owner September 11, 2024 16:11
Copy link
Contributor

API Changes

no api changes detected

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No key issues to review

Copy link
Contributor

github-actions bot commented Sep 11, 2024

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Conditionally replace the endpoint in ListenPath to maintain flexibility

Ensure that the replacement of the endpoint in apidef.Proxy.ListenPath is
conditional based on the environment, to avoid hardcoding test environments and
allow flexibility in testing against different endpoints.

tests/regression/regression_test.go [30]

-apidef.Proxy.ListenPath = strings.ReplaceAll(apidef.Proxy.ListenPath, "httpbin.org", "127.0.0.1:3123")
+if strings.Contains(apidef.Proxy.ListenPath, "httpbin.org") {
+    apidef.Proxy.ListenPath = strings.ReplaceAll(apidef.Proxy.ListenPath, "httpbin.org", "127.0.0.1:3123")
+}
 
Suggestion importance[1-10]: 7

Why: The suggestion to conditionally replace the endpoint in ListenPath adds flexibility and avoids hardcoding, which is beneficial for testing in different environments. This improvement enhances the maintainability and adaptability of the test code.

7
Best practice
Change cleanup registration to use defer for more predictable execution

Replace t.Cleanup(ts.Close) with defer ts.Close() to ensure that the cleanup
operation is registered immediately after the test server starts. This change makes
the cleanup behavior more predictable, especially if the test execution is stopped
before reaching the t.Cleanup call.

tests/regression/issue_10104_test.go [13]

-t.Cleanup(ts.Close)
+defer ts.Close()
 
Suggestion importance[1-10]: 5

Why: The suggestion to use defer instead of t.Cleanup is valid as it ensures immediate registration of the cleanup operation, which can be beneficial for predictability. However, t.Cleanup is designed for test cleanup and is a valid approach, so the change is not crucial.

5
Use defer for cleanup in nested tests to prevent resource leakage

Replace t.Cleanup(ts.Close) with defer ts.Close() in the nested test run to ensure
that resources are cleaned up immediately after the test case finishes, which is
crucial in nested test structures to prevent resource leakage between test cases.

tests/regression/issue_11585_test.go [20]

-t.Cleanup(ts.Close)
+defer ts.Close()
 
Suggestion importance[1-10]: 5

Why: The suggestion to use defer in nested tests is reasonable to ensure cleanup occurs immediately after the test case finishes, which can help prevent resource leakage. However, t.Cleanup is also a valid method for managing cleanup, so the suggestion is not critical.

5
Use defer to ensure immediate cleanup after tests

Replace t.Cleanup(ts.Close) with defer ts.Close() in the test case to ensure that
the server is closed immediately after the test function exits, which is critical
for tests involving multiple configurations or states.

tests/regression/issue_11806_test.go [28]

-t.Cleanup(ts.Close)
+defer ts.Close()
 
Suggestion importance[1-10]: 5

Why: Using defer for cleanup is a good practice to ensure resources are released immediately after the test function exits. However, t.Cleanup is also appropriate for test cleanup, so the suggestion is not essential.

5

Copy link

@titpetric titpetric changed the title Test/ci improvements [TT-13048] Flaky test due to using httpbin org, improvements Sep 11, 2024
@titpetric titpetric enabled auto-merge (squash) September 11, 2024 17:02
@titpetric titpetric merged commit 9091920 into master Sep 11, 2024
27 checks passed
@titpetric titpetric deleted the test/ci-improvements branch September 11, 2024 20:09
@titpetric
Copy link
Contributor Author

/release to release-5.3

Copy link

tykbot bot commented Sep 12, 2024

Working on it! Note that it can take a few minutes.

tykbot bot pushed a commit that referenced this pull request Sep 12, 2024
Using httpbin.org is coupling our CI to their SLA, causing failing
tests.

Test only change for:

- testing httpbin exposed on 3123
- fixes tests/regression flakyness due to httpbin.org usage
- t.Cleanup cleanup instead of defering server close in area (limited
scope)

https://tyktech.atlassian.net/browse/TT-13048

---------

Co-authored-by: Tit Petric <[email protected]>
(cherry picked from commit 9091920)
Copy link

tykbot bot commented Sep 12, 2024

@titpetric Succesfully merged PR

buger added a commit that referenced this pull request Sep 12, 2024
…g, improvements (#6504)

[TT-13048] Flaky test due to using httpbin org, improvements (#6504)

Using httpbin.org is coupling our CI to their SLA, causing failing
tests.

Test only change for:

- testing httpbin exposed on 3123
- fixes tests/regression flakyness due to httpbin.org usage
- t.Cleanup cleanup instead of defering server close in area (limited
scope)

https://tyktech.atlassian.net/browse/TT-13048

---------

Co-authored-by: Tit Petric <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants