test(e2e): filter out retracted version #4129
Conversation
rootulp
requested review from
cmwaters,
ninabarbakadze and
evan-forbes
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
test/e2e/testnet/versions.go (1)
127-135: LGTM with a minor optimization suggestionThe implementation is clean and follows the existing patterns. Consider using
strings.Compareor implementing a custom comparator method for Version structs if more complex version comparison logic is needed in the future.func (v VersionSet) FilterOut(retracted Version) VersionSet { output := make(VersionSet, 0, len(v)) for _, version := range v { - if version != retracted { + if !version.equals(retracted) { // Future-proof with a dedicated comparison method output = append(output, version) } } return output }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/e2e/minor_version_compatibility.go(2 hunks)test/e2e/testnet/versions.go(1 hunks)
🔇 Additional comments (2)
test/e2e/minor_version_compatibility.go (2)
26-27: LGTM: Well-documented constant declaration
The constant declaration clearly explains its purpose through the comment.
38-41: Verify version filtering logic
The implementation correctly filters out the retracted version and includes v3 versions as per PR objectives. Let's verify the filtering behavior.
✅ Verification successful
Based on the gathered information, I can now provide a final response about the version filtering logic verification:
Version filtering implementation is correct and properly structured
The code correctly implements the version filtering requirements:
FilterMajorselects versions matching a specific major versionFilterOutReleaseCandidatesremoves RC versionsFilterOutexcludes the specified retracted version (v1.8.0)- The filtering chain properly concatenates v1, v2, and v3 versions while excluding RCs and the retracted version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the version filtering excludes v1.8.0 and includes v3 versions
# Test: Check if v1.8.0 is properly excluded and v3 versions are included
echo "Running MinorVersionCompatibility test..."
rg "Running MinorVersionCompatibility test with versions" -A 1
Length of output: 157
Script:
#!/bin/bash
# Let's analyze the version filtering logic more comprehensively
# Check the implementation of version filtering functions
ast-grep --pattern 'func FilterMajor($$$) {
$$$
}'
# Look for the retracted versions definition
rg "retracted.*:=" -A 2
# Check the test implementation
rg "func TestMinorVersionCompatibility" -A 10
# Look for version parsing implementation
ast-grep --pattern 'func ParseVersions($$$) {
$$$
}'
Length of output: 428
Script:
#!/bin/bash
# Let's check the version filtering implementation and test setup
# Look for FilterMajor implementation with a more specific pattern
ast-grep --pattern '($_ *Version) FilterMajor($$$) *Version {
$$$
}'
# Check the FilterOutReleaseCandidates implementation
ast-grep --pattern '($_ *Version) FilterOutReleaseCandidates($$$) *Version {
$$$
}'
# Check the FilterOut implementation
ast-grep --pattern '($_ *Version) FilterOut($$$) *Version {
$$$
}'
# Look for the retractedCelestiaApp definition
rg "retractedCelestiaApp.*=" -B 2 -A 2
# Check the test setup and version string
rg "versionStr.*:=" -B 2 -A 2
Length of output: 2067
Script:
#!/bin/bash
# Let's check the getAllVersions implementation and the actual version filtering logic
# Look for getAllVersions implementation
ast-grep --pattern 'func getAllVersions($$$) {
$$$
}'
# Check the Version type definition and methods
rg "type Version" -A 5
# Check for any version-related constants or configurations
rg "const.*Version" -A 2
# Look for the actual test execution logs
rg "MinorVersionCompatibility.*versions:" --ignore-case
Length of output: 40234
Script:
#!/bin/bash
# Let's check the implementation of version filtering functions and the test setup
# Look for the implementation of FilterMajor and other version filtering methods
rg "func.*Filter" test/e2e/testnet/versions.go -A 5
# Check the test implementation and setup
rg "func TestMinorVersionCompatibility" test/e2e/minor_version_compatibility.go -A 20
# Look for getAllVersions implementation
rg "func getAllVersions" test/e2e/minor_version_compatibility.go -A 10
# Check for any version-related constants in the test
rg "const.*Version" test/e2e/minor_version_compatibility.go
Length of output: 1163
| retracted, ok := testnet.ParseVersion(retractedCelestiaApp) | ||
| if !ok { | ||
| logger.Fatal("failed to parse retracted version") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling
The current implementation uses logger.Fatal which will terminate the program. Consider returning an error instead to allow graceful handling by the caller.
- if !ok {
- logger.Fatal("failed to parse retracted version")
- }
+ if !ok {
+ return fmt.Errorf("failed to parse retracted version %s", retractedCelestiaApp)
+ }Committable suggestion skipped: line range outside the PR's diff.
|
This all runs the v1 state machine iiuc. IMO we should only have minor compatibility tests for the latest major version or better still we could come up with a larger test that does both coordinated major version upgrades and spontaneous localized minor version upgrades |
Closes #4116
Testing
test-e2e2024/12/13 10:44:02 === RUN MinorVersionCompatibility test-e2e2024/12/13 10:44:02 Running MinorVersionCompatibility test with versions v1.0.0 v1.1.0 v1.10.0 v1.10.1 v1.11.0 v1.12.0 v1.13.0 v1.14.0 v1.2.0 v1.3.0 v1.3.0 v1.4.0 v1.5.0 v1.6.0 v1.7.0 v1.9.0 v2.0.0 v2.1.0 v2.1.1 v2.1.2 v2.2.0 v2.2.0 v2.2.0 v2.3.0 v2.3.0 v2.3.1 v2.3.1 v3.0.0 v3.0.0 v3.0.0 v3.0.1 v3.0.1 v3.0.1 v3.0.2 v3.0.2 v3.0.2