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

Improve tests #3541

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Improve tests #3541

merged 2 commits into from
Oct 3, 2023

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Oct 2, 2023

1. Explain what the PR does

a2ec86e chore(test): enhance tests using t.Parallel()

Utilize t.Parallel() in unit tests to enable concurrent execution. This
takes advantage of multiple CPU cores, reducing the overall test suite
runtime.

Critically, this practice elevates the detection of insidious race
conditions and state leakages—issues that often remain concealed during
serial test execution.

9b6d344 chore(Makefile): enable '-shuffle on' for random

By using '-shuffle on' we can run the tests in a random order, which
helps us to find any unintentional inter-test dependencies or state
leakages.

2. Explain how to test it

make test-unit
make test-types
sudo make test-integration

3. Other comments

Currently, the use of t.Parallel() doesn't affect the performance of the tests (slight difference):

main:
make test-unit  43,27s user 11,96s system 320% cpu 17,249 total

PR:
make test-unit  43,74s user 11,34s system 325% cpu 16,944 total

By using '-shuffle on' we can run the tests in a random order, which
helps us to find any unintentional inter-test dependencies or state
leakages.
Utilize t.Parallel() in unit tests to enable concurrent execution. This
takes advantage of multiple CPU cores, reducing the overall test suite
runtime.

Critically, this practice elevates the detection of insidious race
conditions and state leakages—issues that often remain concealed during
serial test execution.
@rafaeldtinoco
Copy link
Contributor

Currently, the use of t.Parallel() doesn't affect the performance of the tests (slight difference):

Where ? In runners or your local environment ? Because if in a SMP environment we aren't benefiting from having it set, we shouldn't set it IMO.

@geyslan
Copy link
Member Author

geyslan commented Oct 3, 2023

Currently, the use of t.Parallel() doesn't affect the performance of the tests (slight difference):

Where ? In runners or your local environment ? Because if in a SMP environment we aren't benefiting from having it set, we shouldn't set it IMO.

Local tests shown 1s of improvement, in average, that's why I put that it didn't affect the performance. On Github workflow is slightly similar. But as I pointed in the commit:

Critically, this practice elevates the detection of insidious race
conditions and state leakages—issues that often remain concealed during
serial test execution.

The main reason for using t.Parallel() is to have more checks (i.e.: DATA RACE) when building unit tests.

@rafaeldtinoco
Copy link
Contributor

The main reason for using t.Parallel() is to have more checks (i.e.: DATA RACE) when building unit tests.

Fair enough.

@rafaeldtinoco rafaeldtinoco merged commit 68aa961 into aquasecurity:main Oct 3, 2023
27 checks passed
@geyslan geyslan deleted the improve-tests branch October 31, 2023 17:58
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