fix: return logs for validator tests #109
Conversation
WalkthroughAdds a feature-gated conditional logging block in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
Comment |
| #[cfg(feature = "unit_test_config")] | ||
| if matches!( | ||
| discriminator, | ||
| DlpDiscriminator::Delegate | ||
| | DlpDiscriminator::CommitState | ||
| | DlpDiscriminator::CommitStateFromBuffer | ||
| | DlpDiscriminator::Finalize | ||
| | DlpDiscriminator::Undelegate | ||
| ) { | ||
| msg!("Processing instruction: {:?}", discriminator); | ||
| } |
There was a problem hiding this comment.
Can we please simplify this by removing the whole if matches!() logic and just writing:
#[cfg(feature = "unit_test_config")]
msg!("Processing instruction: {:?}", discriminator);And also remove the msg! line from slow_process_instruction(), so it isn’t logged twice.
However, this change will cause 4 tests to fail in this repo, since they ensure that the CU stays within specific limits ... but this innocent-looking change increases the CU by 413 units. 😄
So.. maybe we can introduce a separate feature flag.. like:
#[cfg(feature = "log-discriminator")]
msg!("Processing instruction: {:?}", discriminator);
// Or `log-testinfo` if more kinds of info are required by the tests. … and enable it only when we actually need discriminator logs. 🤔
There was a problem hiding this comment.
If we need this in production code, maybe we can write a more optimized log utility just to print a preformatted-discriminator and pay 114 CU instead of 413 CU.
There was a problem hiding this comment.
but this innocent-looking change increases the CU by 413 units. 😄
This shall be only in test mode, or do we deploy dlp compiled with unit_test_config ? I would think not. @GabrielePicco
I think if we can it would be better to avoid adding extra feature, unit_test_config already relates to tests and affects cu usage for example in load_program_upgrade_authority. Since its needed in validator tests only i think unit_test_config usage make sense as well, or we can rename it to test_config to avoid confusion
There was a problem hiding this comment.
And also remove the msg! line from slow_process_instruction(), so it isn’t logged twice.
Then we would lose that log in release, while old dlp had it. Not sure if anyone rely on those logs tho @GabrielePicco.
The reason I made it this way:
- keep old(current) implementation intact
- avoid double logging
- enable only for tests
There was a problem hiding this comment.
I think if we can it would be better to avoid adding extra feature, unit_test_config already relates to tests and affects cu usage for example in load_program_upgrade_authority
I suggested introducing a new flag, because otherwise the tests in this repo will continue to fail as they also ensure CUs stays within specific limits. Please check why the CI is failing!
Please see my previous comment again for the details.
There was a problem hiding this comment.
Anyway, now it's up to you and @GabrielePicco to decide. I guess I have shared my thoughts.
There was a problem hiding this comment.
There's no confusion. My point isn’t about optimization level, but about configuration scope and intent. unit_test_config already represents a test-only build variant that enables additional logic specifically for validator and unit tests.
Now, measuring CU on a test build is an invalid measurement by definition. If we want to do things properly, the CU measurement should happen on the production version, which isn’t compiled with unit_test_config. Introducing another flag just to make these measurements work doesn’t fix the underlying issue — it only adds another layer of conditional code that diverges from what’s actually deployed.
There was a problem hiding this comment.
We shouldn’t confuse testing and CU or performance measurements. These serve different purposes.
We should fix an underlying issue - our measurments happen on a test build
There was a problem hiding this comment.
We shouldn’t confuse testing and CU or performance measurements.
performance measurements are part of the testing as well.
The testing is not about correctness only, it is also about how quickly something works, which is why I have added this:
const consumedLog = tx.meta.logMessages.find((m) =>
m.includes("DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh consumed")
);
const consumedCU = parseInt(consumedLog.split(" **").at(3));
assert.isAtMost(
consumedCU,
18500,
"delegate instruction must consume less than 18500"
);You don't call it a test?
To get better and more accurate CU, we can build it in "released" mode with unit_test_config (that is also why this flag should have minimal code under it). If not that, some other flag — either way we introduce a flag.
There was a problem hiding this comment.
No need to make a big deal out of such a small detail.
We don’t need logs in production. We can feature-flag all logs with a “logging” feature to have more granular control. This would work for both the validator and CU tests.
I agree that the CU tests aren’t fully accurate; we can switch to something like Mollusk benchmarking
in the future, but it’s good enough for now.
| #[cfg(feature = "unit_test_config")] | ||
| if matches!( | ||
| discriminator, | ||
| DlpDiscriminator::Delegate | ||
| | DlpDiscriminator::CommitState | ||
| | DlpDiscriminator::CommitStateFromBuffer | ||
| | DlpDiscriminator::Finalize | ||
| | DlpDiscriminator::Undelegate | ||
| ) { | ||
| msg!("Processing instruction: {:?}", discriminator); | ||
| } |
There was a problem hiding this comment.
No need to make a big deal out of such a small detail.
We don’t need logs in production. We can feature-flag all logs with a “logging” feature to have more granular control. This would work for both the validator and CU tests.
I agree that the CU tests aren’t fully accurate; we can switch to something like Mollusk benchmarking
in the future, but it’s good enough for now.
3 participants
Problem
What problem are you trying to solve?
Some of the tests in validator rely on logs by dlp. Those logs not longer present in fast branch
Solution
How did you solve the problem?
Adding logs back under
unit_test_configfeatureBefore & 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