-
Notifications
You must be signed in to change notification settings - Fork 618
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
feat: mutation test script improvements #2436
Conversation
scripts/mutation-test.sh
Outdated
# * ignore test and Protobuf files | ||
MUTATION_SOURCES=$(find ./x -type f \( -path '*/keeper/*' -or -path '*/types/*' \) \( -name '*.go' -and -not -name '*_test.go' -and -not -name '*pb*' \)) | ||
MUTATION_SOURCES=$(find ./x -type f \( -path '*/keeper/*' -or -path '*/types/*' -or -path '*' \) \( -name '*.go' -and -not -name '*_test.go' -and -not -name '*pb*' \)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are allowing any path according to -or -path '*'
? Does it make sense to not apply any path filtering then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof I meant root directory files but I didn't think that included the folders themselves, let me look into this, good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Most recent commit should solve this problem by specifying a max depth after pulling what we want from keeper and types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we excluding paths deeper than 2? Is the goal to exclude files in the client
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want keeper folder and types folder which is the first cmd. Then the second command starts in the x folder, and two up would be the root module folder (because x counts as one) so this gives us any root folder files for the selected modules (I used prints to ensure this works as intended)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, why do we only want the root files of each module? What about pool-model packages and alike?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I didn't want to drift too far away from the original implementation since I lacked context on mutation testing. In fact I would have completely left it alone if twap had the same structure as the other modules. You might be right though, we should probably test this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, What does the output of make look like now?
For ex: make test-mutation MODULES=tokenfactory
@stackman27 now that I have uncommented the lines this should make more sense, sorry about that, was debugging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Related To: #2426
What is the purpose of the change
We were previously hardcoding which module to run mutation tests on. This PR now allows for a comma separated
MODULES
argument to be given to the mutation test script and allows it to run all desired modules.Example:
make test-mutation MODULES=twap
OR
make test-mutation MODULES=twap,superfluid
Leaving
MODULES
blank assumes all modules are desired.Brief Changelog
Testing and Verifying
This change is already covered by existing tests
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no