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

Added several memory checks to CI #2173

Merged
merged 1 commit into from
Jun 11, 2023
Merged

Conversation

ggtakec
Copy link
Member

@ggtakec ggtakec commented May 28, 2023

Relevant Issue (if applicable)

#2172

Details

I added some memory checking to CI process as same as run_tests_using_sanitizers.sh.
Each test in run_tests_using_sanitizers.sh seem best to be a CI Job because all of test several patterns require execution from the build.
The testing base container is Ubuntu:22.04.

This PR is first submitted as a Draft.

Copy link
Member

@gaul gaul left a comment

Choose a reason for hiding this comment

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

I am really excited for this change which should prevent many regressions.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@ggtakec
Copy link
Member Author

ggtakec commented May 29, 2023

@gaul I changed some and squash code, please re-review it.
Thanks.

  • Sorry for my fault, re-checnge code, please wait a moment.

@ggtakec ggtakec force-pushed the memory_test_ci branch 3 times, most recently from 525f119 to 609660c Compare May 29, 2023 13:48
@ggtakec
Copy link
Member Author

ggtakec commented May 29, 2023

I changed the valgrind check to -O1, but it took 1H.
I feel like this test should be disabled.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@ggtakec
Copy link
Member Author

ggtakec commented May 30, 2023

I added -O1, but it doesn't seem to have any effect.
When testing with valgrind, one way is to narrow down the test cases to be executed(currently executed all with ALL_TESTS=1).

@gaul
Copy link
Member

gaul commented May 30, 2023

I added -O1, but it doesn't seem to have any effect.
When testing with valgrind, one way is to narrow down the test cases to be executed(currently executed all with ALL_TESTS=1).

Let's remove valgrind for now and just go with the tests that are successful? The sanitizers have good coverage.

@ggtakec
Copy link
Member Author

ggtakec commented Jun 10, 2023

@gaul
Stopped running valgrind tests.
(I tested it by excluding ALL_TEST=1, but decided to comment it out because it still takes time.)

I post this as a PR as not Draft.
For tests that are currently commented out, let's allow them to be added in the future.

@ggtakec ggtakec marked this pull request as ready for review June 10, 2023 02:13
@gaul gaul merged commit 7c9cf84 into s3fs-fuse:master Jun 11, 2023
16 checks passed
@gaul
Copy link
Member

gaul commented Jun 11, 2023

Thanks for integrating this! We have had several tsan regressions over the years and it will be nice to catch these during PR phase in the future.

@ggtakec ggtakec deleted the memory_test_ci branch June 12, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants