-
Notifications
You must be signed in to change notification settings - Fork 6
Integration tests use prod version of dlp to test against #115
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new production build step for cargo build-sbf was added to the CI workflow. Debug logging was introduced for instruction processing variants. Test delegation configuration was updated to use explicit validator and commit frequency values. Minor indentation cleanup was applied. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/integration/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/run-tests.yml(1 hunks)src/lib.rs(1 hunks)src/processor/fast/finalize.rs(1 hunks)tests/integration/programs/test-delegation/src/lib.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: install
🔇 Additional comments (4)
.github/workflows/run-tests.yml (1)
88-91: LGTM! Production build step correctly positioned.This new step ensures integration tests run against the production build (without
unit_test_configfeature flag), while unit tests continue to use the debug configuration. The positioning is correct: it runs after unit tests and before cleanup, ensuring the productiondlp.soartifact is what gets preserved for integration tests.src/lib.rs (1)
64-74: LGTM! Debug logging appropriately gated.The conditional logging for fast-path instructions is correctly gated by the
unit_test_configfeature flag, ensuring it only appears during unit tests and not in production builds. This aligns well with the PR objective of differentiating between test and production configurations.tests/integration/programs/test-delegation/src/lib.rs (2)
15-15: LGTM! Import added for explicit configuration.The import of
DelegateAccountArgsis necessary to access the default commit frequency value in the explicitDelegateConfigconstruction below.
40-43: I need to examine the full test file to see if there's test setup code and check for test configuration.No verification needed—the hardcoded pubkey is valid test data.
The validator pubkey
"tEsT3eV6RFCWs1BZ7AXTzasHqTtMnMLCB2tjQ42TDXD"is a valid Solana pubkey format (base58-encoded 32 bytes, 32-44 characters long). The naming convention ("tEsT" prefix) and consistent use across lines 42, 55, and 63 indicate this is intentional test data. Thepubkey!macro validates the format at compile-time. Whether the validator account exists during test execution depends on the test setup configuration (Anchor.toml or programmatic validator initialization), which is separate from the code change itself and should be verified during test execution.
75fde9a to
fa8e616
Compare
GabrielePicco
left a comment
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.
LGTM, assuming few comments are addressed before merging
| DelegateConfig::default(), | ||
| DelegateConfig { | ||
| commit_frequency_ms: DelegateAccountArgs::default().commit_frequency_ms, | ||
| validator: Some(pubkey!("tEsT3eV6RFCWs1BZ7AXTzasHqTtMnMLCB2tjQ42TDXD")), |
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.
Should use a consts instead of hardcoding and repeating the validator pubkey
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.
Replaced
src/lib.rs
Outdated
| | DlpDiscriminator::Finalize | ||
| | DlpDiscriminator::Undelegate | ||
| ) { | ||
| msg!("Processing instruction: {:?}", discriminator); |
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.
This change is unrelated to the PR. As mentioned in PR #109 I suggest to move all logs under a logging feature and to simplify it:
#[cfg(feature = "logging")]
msg!("Processing instruction: {:?}", discriminator);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.
Removed this change, redirected PR into main
c5107ed to
471d2cc
Compare
|
I liked this solution. Great work, @taco-paco . :-) |
Problem
What problem are you trying to solve?
Solution
How did you solve the problem?
Before & After Screenshots
Insert screenshots of example code output
BEFORE:
[insert screenshot here]
AFTER:
[insert screenshot here]
Other changes (e.g. bug fixes, small refactors)
Deploy Notes
Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.
New scripts:
script: script detailsNew dependencies:
dependency: dependency detailsSummary by CodeRabbit
Tests
Chores