test(fuzz): fix broken OSS-Fuz build #21940
Conversation
Signed-off-by: Adam Korczynski <Adam@adalogics.com>
📝 WalkthroughWalkthroughThe changes involve the removal of several test functions from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildScript
participant GoEnv
participant FuzzingComponents
User->>BuildScript: Initiate build process
BuildScript->>GoEnv: Install Go 1.23.1
BuildScript->>FuzzingComponents: Apply patches and reorganize files
BuildScript->>GoEnv: Run go mod tidy
BuildScript->>User: Build complete
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
fuzz/fuzz.patch (1)
1-81: Summary of changes and recommendationsThe changes in this patch involve the removal of several test functions from
types/address_test.go, includingTestAddressTestSuite,TestBech32ifyAddressBytes, andTestMustBech32ifyAddressBytes. While these removals may be necessary to fix the OSS-Fuzz build as mentioned in the PR objectives, they could potentially impact the overall test coverage and reliability of the codebase.Recommendations:
- Document the reasons for removing these tests in the code or commit message.
- Ensure that the functionalities tested by the removed functions are covered elsewhere in the codebase.
- Consider adding minimal tests that don't interfere with the OSS-Fuzz build but still provide some coverage for the affected functions.
- Monitor the impact of these changes on the overall test coverage and system stability.
- Plan for reintroducing comprehensive tests for these functions in the future, possibly as part of the mentioned improvements to the OSS-Fuzz infrastructure.
As the PR author mentioned that this is a temporary, "hacky" solution, it's crucial to create a follow-up task or issue to revisit these changes and restore proper test coverage once the OSS-Fuzz infrastructure improvements are implemented.
fuzz/oss-fuzz-build.sh (1)
17-17: Avoid moving binaries to/root/go/bin/; use a user-local bin directoryMoving the
go-118-fuzz-buildbinary to/root/go/bin/requires root privileges and may not be necessary. It's better to place binaries in a user-local directory, such as$HOME/go/binor$SRC/go/bin, to avoid permission issues and improve portability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- fuzz/fuzz.patch (1 hunks)
- fuzz/oss-fuzz-build.sh (2 hunks)
🔇 Additional comments (3)
fuzz/fuzz.patch (1)
20-48: Verify coverage for Bech32ifyAddressBytes functionalityThe removal of
TestBech32ifyAddressBytesfunction eliminates tests for theBech32ifyAddressBytesfunctionality. This could potentially leave this critical function untested, risking future undetected bugs.Let's verify if this functionality is tested elsewhere:
#!/bin/bash # Description: Check for other tests covering Bech32ifyAddressBytes functionality # Search for other tests of Bech32ifyAddressBytes echo "Searching for other tests of Bech32ifyAddressBytes:" rg --type go 'func Test.*Bech32ifyAddressBytes' # Look for usage of Bech32ifyAddressBytes in the codebase echo "Checking usage of Bech32ifyAddressBytes:" rg --type go 'Bech32ifyAddressBytes'If no other tests are found, consider adding a minimal test for
Bech32ifyAddressBytesthat doesn't interfere with the OSS-Fuzz build. Alternatively, document why this test was removed and how the functionality is ensured to work correctly without these specific tests.fuzz/oss-fuzz-build.sh (2)
21-23: Verify that moving test files does not impact test executionMoving test files to
types/fuzzingand changing their package fromtypes_testtofuzzingmay affect how tests are discovered and executed. Ensure that the tests in the new location are still being run as part of your test suite.
59-59: Verify thatFuzzBech32AccAddrConsistencyYAMLis properly definedEnsure that the function
FuzzBech32AccAddrConsistencyYAMLexists in thetypes/fuzzingpackage and is correctly implemented for fuzzing purposes.You can run the following script to confirm the function's existence:
✅ Verification successful
[/run_scripts]
<!-- [monologue] [task verify_comment_1/1] [observations] - The shell script successfully found the definition of `FuzzBech32AccAddrConsistencyYAML` in `types/address_fuzz_test.go`. - The function signature `func FuzzBech32AccAddrConsistencyYAML(f *testing.F)` indicates it is set up for fuzz testing. - The implementation includes a test case addition, which aligns with fuzzing purposes. [analysis] - The function exists and is correctly defined for fuzzing within the `types` package. - The initial concern about the function's existence is addressed. - No additional verification is necessary unless there's a requirement to ensure it's in a specific subdirectory. [actions] - Conclude that the verification is successful. -->FuzzBech32AccAddrConsistencyYAML function is properly defined
The function
FuzzBech32AccAddrConsistencyYAMLwas found intypes/address_fuzz_test.goand is correctly implemented for fuzzing purposes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the definition of FuzzBech32AccAddrConsistencyYAML in the types/fuzzing package. # Test: Expect to find the function definition in Go files within the types/fuzzing directory. rg --type go -A 5 -B 5 'func FuzzBech32AccAddrConsistencyYAML' $SRC/cosmos-sdk/types/fuzzingLength of output: 156
Script:
#!/bin/bash # Description: Recursively search for the definition of FuzzBech32AccAddrConsistencyYAML across all Go files in the repository. # Test: Expect to find the function definition in relevant Go files. rg --type go -A 5 -B 5 'func FuzzBech32AccAddrConsistencyYAML' .Length of output: 666
| -func (s *addressTestSuite) TestMustBech32ifyAddressBytes() { | ||
| - type args struct { | ||
| - prefix string | ||
| - bs []byte | ||
| - } | ||
| - tests := []struct { | ||
| - name string | ||
| - args args | ||
| - want string | ||
| - wantPanic bool | ||
| - }{ | ||
| - {"empty address", args{"prefixa", []byte{}}, "", false}, | ||
| - {"empty prefix", args{"", addr20byte}, "", true}, | ||
| - {"10-byte address", args{"prefixa", addr10byte}, "prefixa1qqqsyqcyq5rqwzqf3953cc", false}, | ||
| - {"10-byte address", args{"prefixb", addr10byte}, "prefixb1qqqsyqcyq5rqwzqf20xxpc", false}, | ||
| - {"20-byte address", args{"prefixa", addr20byte}, "prefixa1qqqsyqcyq5rqwzqfpg9scrgwpugpzysn7hzdtn", false}, | ||
| - {"20-byte address", args{"prefixb", addr20byte}, "prefixb1qqqsyqcyq5rqwzqfpg9scrgwpugpzysnrujsuw", false}, | ||
| - } | ||
| - for _, tt := range tests { | ||
| - s.T().Run(tt.name, func(t *testing.T) { | ||
| - if tt.wantPanic { | ||
| - require.Panics(t, func() { types.MustBech32ifyAddressBytes(tt.args.prefix, tt.args.bs) }) | ||
| - return | ||
| - } | ||
| - require.Equal(t, tt.want, types.MustBech32ifyAddressBytes(tt.args.prefix, tt.args.bs)) | ||
| - }) | ||
| - } | ||
| -} |
There was a problem hiding this comment.
💡 Codebase verification
Additional tests for MustBech32ifyAddressBytes are needed
The removal of TestMustBech32ifyAddressBytes reduces the test coverage for the MustBech32ifyAddressBytes function, particularly its panic conditions. While there is usage in client/keys/show_test.go, it may not comprehensively cover all test cases previously handled.
- Ensure that all critical scenarios, including different prefixes, byte lengths, and panic conditions, are adequately tested.
- If necessary, reintroduce or create new test functions to maintain full coverage of
MustBech32ifyAddressBytes.
🔗 Analysis chain
Verify coverage for MustBech32ifyAddressBytes functionality
The removal of TestMustBech32ifyAddressBytes function eliminates tests for the MustBech32ifyAddressBytes functionality, including its panic behavior. This could potentially leave this critical function untested, risking unexpected runtime errors if the function's panic conditions are not properly maintained.
Let's verify if this functionality is tested elsewhere:
If no other tests are found, consider adding a minimal test for MustBech32ifyAddressBytes that doesn't interfere with the OSS-Fuzz build, especially focusing on its panic behavior. Alternatively, document why this test was removed and how the functionality, particularly the panic conditions, is ensured to work correctly without these specific tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other tests covering MustBech32ifyAddressBytes functionality
# Search for other tests of MustBech32ifyAddressBytes
echo "Searching for other tests of MustBech32ifyAddressBytes:"
rg --type go 'func Test.*MustBech32ifyAddressBytes'
# Look for usage of MustBech32ifyAddressBytes in the codebase
echo "Checking usage of MustBech32ifyAddressBytes:"
rg --type go 'MustBech32ifyAddressBytes'
Length of output: 943
| -func TestAddressTestSuite(t *testing.T) { | ||
| - suite.Run(t, new(addressTestSuite)) | ||
| -} |
There was a problem hiding this comment.
💡 Codebase verification
Potential Test Coverage Reduction Due to Removal of Test Suite Runner
The TestAddressTestSuite function has been removed, and no alternative test suite runners were found for address-related tests. While individual tests like TestBech32ifyAddressBytes still exist in types/address_test.go, their execution may be compromised without a suite runner, potentially reducing overall test coverage.
- Ensure that individual tests are invoked through other test runners or frameworks.
- Consider reintroducing a suite runner or alternative mechanism to execute these tests.
- Add comments to clarify the rationale behind removing the suite runner and how test coverage is maintained.
🔗 Analysis chain
Verify the impact of removing TestAddressTestSuite
The removal of TestAddressTestSuite function, which was responsible for running the entire address test suite, could significantly impact the test coverage. While this might be necessary to fix the OSS-Fuzz build as per the PR objectives, we need to ensure that critical tests are still being run elsewhere.
Let's verify if there are other test suite runners or if these tests are covered elsewhere:
Consider adding a comment explaining why these tests were removed and how the functionality is being tested now, if applicable. This will help maintain clarity for future developers working on this code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other test suite runners and coverage of address tests
# Search for other potential test suite runners
echo "Searching for other test suite runners:"
rg --type go 'func Test.*Suite\(' -g '!*_test.go'
# Check for other files that might be testing address functionality
echo "Checking for other address test files:"
fd -e go | rg 'address.*test'
# Look for any TODO or FIXME comments related to testing
echo "Searching for TODO/FIXME comments related to testing:"
rg -i 'todo|fixme' -g '*test*.go'
Length of output: 7093
| set -o nounset | ||
| set -o pipefail | ||
| set -o errexit | ||
| set -x |
There was a problem hiding this comment.
Remove set -x to prevent verbose logging in production
The set -x command enables shell debugging by printing each command before execution. This can clutter logs and potentially expose sensitive information. It's recommended to remove set -x or conditionally enable it only during debugging sessions.
| rm -rf /root/.go && tar -C $SRC/new-go/ -xzf go1.23.1.linux-amd64.tar.gz | ||
| mv $SRC/new-go/go /root/.go |
There was a problem hiding this comment.
Avoid using rm -rf /root/.go; consider safer installation methods
Using rm -rf /root/.go is risky and can lead to accidental deletion if misused. Additionally, modifying files in /root requires elevated privileges and may affect other processes. Consider installing Go in a local directory within your workspace or using the existing Go installation available in the environment.
| rm $SRC/cosmos-sdk/math/dec_internal_test.go | ||
| rm $SRC/cosmos-sdk/math/int_internal_test.go | ||
| rm $SRC/cosmos-sdk/math/uint_internal_test.go | ||
| mv $SRC/cosmos-sdk/types/fuzz_test.go $SRC/cosmos-sdk/types/fuzz.go | ||
| rm $SRC/cosmos-sdk/types/*_test.go | ||
| mv $SRC/cosmos-sdk/types/fuzz.go $SRC/cosmos-sdk/types/fuzz_test.go |
There was a problem hiding this comment.
Assess the impact of removing test files on code coverage
Removing test files like dec_internal_test.go, int_internal_test.go, uint_internal_test.go, and all *_test.go files in the types directory could significantly reduce test coverage. This might lead to undetected bugs and decreased code reliability. Please evaluate the necessity of deleting these tests and consider retaining critical ones.
Description
This fixes the broken OSS-Fuzz build. It is quite a hacky fix and the goal is to enable OSS-Fuzz to build the fuzzers.
I plan to work on the OSS-Fuzz infrastructure side to make this less hacky, but it will take a few weeks, so I suggest we fix the build so the fuzzers at least run.
I will keep an eye on the build in case it breaks to minimize downtime.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Bug Fixes
New Features
Chores