Skip to content

Conversation

@jacinta-stacks
Copy link
Contributor

@jacinta-stacks jacinta-stacks commented Nov 18, 2025

I am not yet convinced that my reasoning about my untested variants is correct. Still looking into it. Especially the AST errors. Having a hard time reasoning out when those would trigger.

Replaces #6690

Closes #6695

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 66.33803% with 239 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.78%. Comparing base (44fdf84) to head (11b5a6d).

Files with missing lines Patch % Lines
stackslib/src/chainstate/tests/runtime_tests.rs 52.55% 204 Missing ⚠️
clarity/src/vm/contexts.rs 61.66% 23 Missing ⚠️
clarity/src/vm/database/clarity_db.rs 90.54% 7 Missing ⚠️
clarity/src/vm/database/sqlite.rs 83.33% 2 Missing ⚠️
pox-locking/src/pox_4.rs 96.96% 2 Missing ⚠️
stackslib/src/chainstate/tests/parse_tests.rs 0.00% 1 Missing ⚠️

❌ Your project check has failed because the head coverage (70.78%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6698      +/-   ##
===========================================
- Coverage    73.67%   70.78%   -2.90%     
===========================================
  Files          577      578       +1     
  Lines       357795   358488     +693     
===========================================
- Hits        263596   253741    -9855     
- Misses       94199   104747   +10548     
Files with missing lines Coverage Δ
clarity-types/src/types/mod.rs 91.73% <ø> (-2.15%) ⬇️
clarity/src/vm/costs/cost_functions.rs 100.00% <ø> (ø)
clarity/src/vm/functions/assets.rs 85.43% <ø> (ø)
clarity/src/vm/variables.rs 96.15% <100.00%> (+2.82%) ⬆️
stacks-common/src/types/mod.rs 73.51% <100.00%> (+0.33%) ⬆️
stackslib/src/chainstate/tests/consensus.rs 85.53% <100.00%> (-5.21%) ⬇️
stackslib/src/chainstate/tests/mod.rs 77.93% <ø> (ø)
stackslib/src/clarity_vm/database/marf.rs 59.07% <ø> (ø)
stackslib/src/chainstate/tests/parse_tests.rs 72.11% <0.00%> (-5.58%) ⬇️
clarity/src/vm/database/sqlite.rs 84.30% <83.33%> (+2.44%) ⬆️
... and 4 more

... and 275 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44fdf84...11b5a6d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@federico-stacks federico-stacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a small remark. I would have approved and let you decide how to handle it, but I also noticed that your commits are no longer signed. Not sure whether that’s due to your local configuration or the change Jesse made to the CI. Just flagging it so you’re aware.

Comment on lines +421 to +434
/// Error: [`RuntimeError::TypeParseFailure`]
/// Caused by: invalid standard principal literal (wrong byte length)
/// Outcome: block accepted.
/// Note: Gets converted into [`clarity::vm::ast::errors::ParseErrorKind::InvalidPrincipalLiteral`]
#[test]
pub fn principal_wrong_byte_length() {
contract_deploy_consensus_test!(
contract_name: "wrong-byte-length",
contract_code: "
;; This literal decodes via c32 but has the wrong byte length
(define-constant my-principal 'S162RK3CHJPCSSK6BM757FW)",
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is practically triggerable from 5 different places, 3 are the parse methods (that are never called at runtime) and 2 are:

  • Environment::eval_raw (only used for testing)
  • Environment::eval_read_only (not sure if it's used during mining/append block)
    Do you think it's possible to try to trigger this from Environment::eval_read_only, if it's reachable? If not I'd direclty mark this Error variant as unrechable

Copy link
Contributor Author

@jacinta-stacks jacinta-stacks Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, some of these runtime errors actually trigger during static analysis and get converted into parse error types. I think it needs to remain. I.e. this error type maybe should be something else (not a runtime error). But until we make that change, this test is needed as the code path itself is reachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that these are not CheckErrorKind tests being triggered at runtime. They are just tests trying to hit all possible paths of RuntimeError which is a sep error type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry for the confusion! I totally understand we're testing RuntimeError variants here. I think I was approaching this from a different angle based on what I did in the other PRs for CheckErrorKind variants.

Your approach here is actually way better and more exhaustive. You're trying to hit all possible code paths where these RuntimeError variants can be reached, which gives us much better coverage of the error type itself.

In my CheckErrorKind tests, I only had 1 test per variant, so I wasn't covering every possible way to trigger it. The key difference is what we're testing:

  • CheckErrorKind tests: Focus on how StacksChainState::process_transaction_payload handles each error variant when it receives it from processing a Clarity tx. i.e., does error variant X lead to block rejection or acceptance?

  • Thse RuntimeError tests: Focus on actually reaching each error variant through the code paths, ensuring the variant is reachable.

So for for this test, you're right, it's reachable (even though it gets converted to ParseErrorKind::InvalidPrincipalLiteral before reaching process_transaction_payload) and correct to keep! Same with arithmetic_zero_n_log_n that eventually wraps into CheckErrorKind::CostComputationFailed.

The thing is, for arithmetic_zero_n_log_n we're still covered because you have other tests that return a pure RuntimeError::Arithmetic to process_transaction_payload. But for principal_wrong_byte_length, we currently don't have a test showing how process_transaction_payload would handle a raw RuntimeError::TypeParseFailure that reaches it without conversion.

Would it make sense to either:

  1. Keep this test as-is (since it proves the RuntimeError variant is reachable), or
  2. Add a separate test that triggers RuntimeError::TypeParseFailure in a way that it actually reaches process_transaction_payload without conversion?

What do you think?

Copy link

@francesco-stacks francesco-stacks Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also do it in a separate PR. Feel free to resolve the conversation

@jacinta-stacks
Copy link
Contributor Author

jacinta-stacks commented Nov 25, 2025

Left a small remark. I would have approved and let you decide how to handle it, but I also noticed that your commits are no longer signed. Not sure whether that’s due to your local configuration or the change Jesse made to the CI. Just flagging it so you’re aware.

EDIT: FIXED

@wileyj
Copy link
Collaborator

wileyj commented Nov 25, 2025

Left a small remark. I would have approved and let you decide how to handle it, but I also noticed that your commits are no longer signed. Not sure whether that’s due to your local configuration or the change Jesse made to the CI. Just flagging it so you’re aware.

My commits are signed but the email assigned to them doesn't match github's email. @wileyj is this a problem?

not directly, but it's a good idea to update the commits with the correct email.

@jacinta-stacks
Copy link
Contributor Author

Apologise in advance. Gotta force push to fix all my signatures!

Copy link

@francesco-stacks francesco-stacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Really thorough work on the coverage here. I just have two points that we can address in a separate PR:

  1. RuntimeError::TypeParseFailure handling: We should decide what to do about this variant. ideally find a way to return that error directly from a contract call/contract initialization so we can verify how process_transaction_payload handles it without conversion to ParseErrorKind.

  2. Testing both contract call AND contract deploy paths: Currently, all tests trigger the runtime errors during a contract call. During real operations, these errors can be triggered in two places:

    • Contract calls
    • Contract initialization during contract deploy (after it passes static analysis)

    I believe it's important that we test both paths, with at least 1 test per variant covering each scenario. This is because the code that handles the returned error is different between a contract deploy and a contract call and, even if it shouldn't, it may treat the same error variant differently depending on where it originated.

    The good news is that duplicating these tests and adapting them for deploy should be fairly straightforward. For example:

    Current (contract call):

    #[test]
    fn to_uint_underflow() {
        contract_call_consensus_test!(
            contract_name: "to-uint-negative",
            contract_code: "
    (define-read-only (trigger-underflow)
      (to-uint -10)
    )",
            function_name: "trigger-underflow",
            function_args: &[],
        );
    }

    Deploy version:

    #[test]
    fn to_uint_underflow_deploy() {
        contract_deploy_consensus_test!(
            contract_name: "to-uint-negative",
            contract_code: "
    (begin
      (to-uint -10)
    )",
        );
    }

Comment on lines +421 to +434
/// Error: [`RuntimeError::TypeParseFailure`]
/// Caused by: invalid standard principal literal (wrong byte length)
/// Outcome: block accepted.
/// Note: Gets converted into [`clarity::vm::ast::errors::ParseErrorKind::InvalidPrincipalLiteral`]
#[test]
pub fn principal_wrong_byte_length() {
contract_deploy_consensus_test!(
contract_name: "wrong-byte-length",
contract_code: "
;; This literal decodes via c32 but has the wrong byte length
(define-constant my-principal 'S162RK3CHJPCSSK6BM757FW)",
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry for the confusion! I totally understand we're testing RuntimeError variants here. I think I was approaching this from a different angle based on what I did in the other PRs for CheckErrorKind variants.

Your approach here is actually way better and more exhaustive. You're trying to hit all possible code paths where these RuntimeError variants can be reached, which gives us much better coverage of the error type itself.

In my CheckErrorKind tests, I only had 1 test per variant, so I wasn't covering every possible way to trigger it. The key difference is what we're testing:

  • CheckErrorKind tests: Focus on how StacksChainState::process_transaction_payload handles each error variant when it receives it from processing a Clarity tx. i.e., does error variant X lead to block rejection or acceptance?

  • Thse RuntimeError tests: Focus on actually reaching each error variant through the code paths, ensuring the variant is reachable.

So for for this test, you're right, it's reachable (even though it gets converted to ParseErrorKind::InvalidPrincipalLiteral before reaching process_transaction_payload) and correct to keep! Same with arithmetic_zero_n_log_n that eventually wraps into CheckErrorKind::CostComputationFailed.

The thing is, for arithmetic_zero_n_log_n we're still covered because you have other tests that return a pure RuntimeError::Arithmetic to process_transaction_payload. But for principal_wrong_byte_length, we currently don't have a test showing how process_transaction_payload would handle a raw RuntimeError::TypeParseFailure that reaches it without conversion.

Would it make sense to either:

  1. Keep this test as-is (since it proves the RuntimeError variant is reachable), or
  2. Add a separate test that triggers RuntimeError::TypeParseFailure in a way that it actually reaches process_transaction_payload without conversion?

What do you think?

Comment on lines +421 to +434
/// Error: [`RuntimeError::TypeParseFailure`]
/// Caused by: invalid standard principal literal (wrong byte length)
/// Outcome: block accepted.
/// Note: Gets converted into [`clarity::vm::ast::errors::ParseErrorKind::InvalidPrincipalLiteral`]
#[test]
pub fn principal_wrong_byte_length() {
contract_deploy_consensus_test!(
contract_name: "wrong-byte-length",
contract_code: "
;; This literal decodes via c32 but has the wrong byte length
(define-constant my-principal 'S162RK3CHJPCSSK6BM757FW)",
);
}
Copy link

@francesco-stacks francesco-stacks Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also do it in a separate PR. Feel free to resolve the conversation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants