chore: reduce the bittwister tests #384
Conversation
|
We should merge this PR in #401 so that we can run the tests on this PR. |
|
I was looking at the runtimes of the bittwister tests in some recent CI and I think we should restructure these tests even more to save more CI time. Currently the test structure is generally the following: func TestX() {
t.Parallel()
// testSetUp
// define test cases
// run test cases
}This structure runs the the high level tests in parallel but runs the test cases sequentially. Normally this is ideal because the test setup is the bulk of the test time and the test cases are not very time intensive. However for these tests it is the reverse, in that the test cases are where the bulk of the time is spent. So we could structure them like this: func TestX() {
t.Parallel()
// Define test cases
// run testcases
t.Run(
t.Parallel()
// Test setup
)
}This is going to duplicate the time of test setup up, but enables running each test case in parallel in addition to running each high level test in parallel. It is worth noting though that this change is optimizing for local CI as the github runners only have a single core I believe so tests never run in parallel anyways. But as it would only increase the overall run time by POC with the Jitter test locally. running the tests with the new parallel set up |
WalkthroughThe changes primarily focus on optimizing and refactoring the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- e2e/basic/bittwister_test.go (4 hunks)
Additional comments not posted (1)
e2e/basic/bittwister_test.go (1)
396-396: The changes in the jitter tests align well with the PR objectives.The modifications to the jitter tests are appropriate and align with the objectives of reducing the number of tests while maintaining essential coverage.
I think that adds a value, however it is tricky to choose the right tolerance. We either have to keep it a large number for most tests or find a formula to calculate it to let some room for the measurement accuracy errors.
t.Run(
t.Parallel()
// Test setup
)if we wanna run those tests in parallel we need to create or clone dedicated instances per test case because currently, bittwister tests uses the same instance to apply traffic shaping. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- e2e/basic/bittwister_test.go (15 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e/basic/bittwister_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
e2e/bittwister/bittwister_test.go (1)
Line range hint
21-115: Well-structured test function with dynamic test case generation. Consider adding more detailed comments explaining the choice of random values and their impact on test robustness.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- e2e/bittwister/bittwister_test.go (4 hunks)
- e2e/bittwister/suite_setup_test.go (1 hunks)
- pkg/instance/instance.go (1 hunks)
- pkg/knuu/knuu.go (1 hunks)
- pkg/knuu/knuu_old.go (1 hunks)
Additional comments not posted (6)
e2e/bittwister/suite_setup_test.go (3)
13-16: TheSuitestruct is well-defined and appropriate for encapsulating the test suite's state.
29-37: The cleanup process inTearDownSuiteis robust, ensuring resources are properly released even if errors occur.
39-41: Standard and correct usage of thesuite.Runto execute the test suite.pkg/knuu/knuu_old.go (1)
99-99: Ensure the use ofcontext.Background()inHandleStopSignalis justified and documented, especially since this file is for backward compatibility.Verification successful
The usage of
context.Background()in the deprecated filespkg/knuu/knuu_old.go,pkg/knuu/preloader_old.go, andpkg/knuu/instance_old.gois justified for maintaining backward compatibility. It ensures that these deprecated functions remain functional without introducing new dependencies or complexities.
pkg/knuu/knuu_old.gopkg/knuu/preloader_old.gopkg/knuu/instance_old.goConsider documenting this rationale to provide clarity for future maintainers.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of context.Background() in deprecated files. # Test: Search for the usage of context.Background() in deprecated files. Expect: Justified usage. rg --type go $'context.Background()' --files-with-matches | xargs grep -l 'This file is deprecated'Length of output: 172
Script:
#!/bin/bash # Extract code snippets around the usage of context.Background() in the identified files. # Extract 5 lines before and after the usage of context.Background() in pkg/knuu/knuu_old.go echo "pkg/knuu/knuu_old.go:" rg -C 5 'context.Background()' pkg/knuu/knuu_old.go # Extract 5 lines before and after the usage of context.Background() in pkg/knuu/preloader_old.go echo "pkg/knuu/preloader_old.go:" rg -C 5 'context.Background()' pkg/knuu/preloader_old.go # Extract 5 lines before and after the usage of context.Background() in pkg/knuu/instance_old.go echo "pkg/knuu/instance_old.go:" rg -C 5 'context.Background()' pkg/knuu/instance_old.goLength of output: 8265
pkg/knuu/knuu.go (1)
172-178: Excellent use of context inHandleStopSignalfor better signal handling and resource cleanup.pkg/instance/instance.go (1)
1094-1106: RefactorWaitInstanceIsRunningto improve reliability.The changes in the
WaitInstanceIsRunningmethod enhance the reliability by periodically checking the instance's state. This is a good practice in asynchronous operations. However, consider adding more detailed logging at each retry to improve traceability and debugging.
Closes #381
Summary by CodeRabbit
Refactor
Chores