Skip to content
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

app: 🤠 address warning #3900

Merged
merged 1 commit into from
Feb 27, 2024
Merged

app: 🤠 address warning #3900

merged 1 commit into from
Feb 27, 2024

Conversation

cratelyn
Copy link
Contributor

addresses this compiler warning:

warning: argument must be of the form: `expected = "error message"`
  --> crates/core/app/tests/spend.rs:97:1
   |
97 | #[cfg_attr(
   | ^
   |
   = note: errors in this attribute were erroneously allowed and will become a hard error in a future release

addresses this compiler warning:

```
warning: argument must be of the form: `expected = "error message"`
  --> crates/core/app/tests/spend.rs:97:1
   |
97 | #[cfg_attr(
   | ^
   |
   = note: errors in this attribute were erroneously allowed and will become a hard error in a future release
```
@cratelyn cratelyn added C-chore Codebase maintenance that doesn't fix bugs or add features, and isn't urgent or blocking. A-testing Area: Relates to testing of Penumbra labels Feb 27, 2024
@cratelyn cratelyn added this to the Sprint 1 milestone Feb 27, 2024
@cratelyn cratelyn self-assigned this Feb 27, 2024
Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

    // 3. Simulate execution of the Spend action
    spend
        .check_stateless(transaction_context)
        .await
        .unwrap_err()
        .to_string()
        .contains("spend proof did not verify");

This should be wrapped in an assert! right?

@cratelyn
Copy link
Contributor Author

    // 3. Simulate execution of the Spend action
    spend
        .check_stateless(transaction_context)
        .await
        .unwrap_err()
        .to_string()
        .contains("spend proof did not verify");

This should be wrapped in an assert! right?

i'm not sure. i didn't write this test, it was added in #3871. i just wanted to address the warning i'm seeing when building main

@@ -96,7 +96,7 @@ async fn spend_happy_path() -> anyhow::Result<()> {
#[tokio::test]
#[cfg_attr(
debug_assertions,
should_panic("assertion failed: cs.is_satisfied().unwrap()")
should_panic = "assertion failed: cs.is_satisfied().unwrap()"
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge deal, but reading the last state of the proof test, I am not sure I understand why we expect this to panic? the attribute makes me think it might be a debug assert nested deep in arkworks? If so we should probably add a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

This is cursed, if you try to verify an invalid proof in debug mode, arkworks will panic with this error. So we expect this to panic in debug mode, in release mode it should error with an invalid spend proof. The way the test is written currently with contains() is a bug

@erwanor
Copy link
Member

erwanor commented Feb 27, 2024

Ah yeah, np I'll ask @avahowell

@cratelyn cratelyn merged commit 628a9b6 into main Feb 27, 2024
6 checks passed
@cratelyn cratelyn deleted the kate/fix-test-warning branch February 27, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: Relates to testing of Penumbra C-chore Codebase maintenance that doesn't fix bugs or add features, and isn't urgent or blocking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants