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

Added assert_ne! test macro #4424

Merged

Conversation

clint419
Copy link
Contributor

@clint419 clint419 commented Nov 20, 2023

@orizi Hi, i added an assert_ne! macro, you see if the pr is necessary, thank you


This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1, all commit messages.
Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @clint419)


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 19 at r1 (raw file):

    fn generate_code(
        &self,
        db: &dyn SyntaxGroup,

and remove the later explicit call.

Suggestion:

trait CompareAssertionPlugin: NamedPlugin {
    const OPERATOR: &'static str;
}

impl NamedPlugin for CompareAssertionPlugin {
    fn generate_code(
        &self,
        db: &dyn SyntaxGroup,

crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 101 at r1 (raw file):

                        );
            "#,
            },

Suggestion:

        let operator = Self::OPERATOR;
        builder.add_modified(RewriteNode::interpolate_patched(
            &formatdoc! {
                r#"
                {{
                    {maybe_assign_lhs}
                    {maybe_assign_rhs}
                    if !(@$lhs_value$ {operator} @$rhs_value$) {{
                        let mut {f}: core::fmt::Formatter = core::traits::Default::default();
                        core::result::ResultTrait::<(), core::fmt::Error>::unwrap(
                            write!({f}, "assertion `{lhs_escaped} {operator} {rhs_escaped}` failed")
                        );
            "#,
            },

Copy link
Contributor Author

@clint419 clint419 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 19 at r1 (raw file):

and remove the later explicit call.

The CompareAssertionPlugin is a trait that does not implement the NamedPlugin


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 101 at r1 (raw file):

                        );
            "#,
            },
  • This is a good suggestion

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @clint419)


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 19 at r1 (raw file):

Previously, clint419 (Clint) wrote…

and remove the later explicit call.

The CompareAssertionPlugin is a trait that does not implement the NamedPlugin

instead:

trait CompareAssertionPlugin {
    const NAME: &'static str;
    const OPERATOR: &'static str;
}

impl<T: CompareAssertionPlugin> NamedPlugin for T {
...

Copy link
Contributor Author

@clint419 clint419 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 19 at r1 (raw file):

Previously, orizi wrote…

instead:

trait CompareAssertionPlugin {
    const NAME: &'static str;
    const OPERATOR: &'static str;
}

impl<T: CompareAssertionPlugin> NamedPlugin for T {
...

The code violates orphan rules

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @clint419)

a discussion (no related file):
add test to corelib/src/test/testing_test.cairo



crates/cairo-lang-test-runner/test_data/lib.cairo line 54 at r2 (raw file):

        let mut contract1 = IBalanceDispatcher { contract_address: address1 };

        assert_eq!(contract0.get(), 100);

revert.


scripts/starknet_test.sh line 6 at r2 (raw file):

    crates/cairo-lang-starknet/cairo_level_tests/ --starknet && \
cargo run --bin cairo-test -- \
    crates/cairo-lang-test-runner/test_data/ --starknet

revert.


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 19 at r1 (raw file):

Previously, clint419 (Clint) wrote…

The code violates orphan rules

you can solve it in the standard way of solving it by using a wrapper.

If you don't want to do this - just make the generate_code a function with generic arg - you get nothing from this trait.

@clint419 clint419 force-pushed the add_assert_ne_macro branch from 05997e9 to ebeb425 Compare November 20, 2023 12:03
Copy link
Contributor Author

@clint419 clint419 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @orizi)


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 19 at r1 (raw file):

you can solve it in the standard way of solving it by using a wrapper.

Yes. can i use macro complete?

Copy link
Contributor Author

@clint419 clint419 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @orizi)


crates/cairo-lang-test-runner/test_data/lib.cairo line 54 at r2 (raw file):

Previously, orizi wrote…

revert.

Done.


scripts/starknet_test.sh line 6 at r2 (raw file):

Previously, orizi wrote…

revert.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @clint419)


crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 13 at r3 (raw file):

use indoc::formatdoc;

trait CompareAssertionPlugin: NamedPlugin {

Doc

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @clint419)

@clint419 clint419 force-pushed the add_assert_ne_macro branch from fde0e81 to d139f41 Compare November 20, 2023 12:30
Copy link
Contributor Author

@clint419 clint419 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

add test to corelib/src/test/testing_test.cairo

Done.



crates/cairo-lang-test-plugin/src/inline_macros/assert.rs line 13 at r3 (raw file):

Previously, orizi wrote…

Doc

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @clint419)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @clint419)

a discussion (no related file):
@gilbens-starkware for 2nd eye.


Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 6 files at r1, 1 of 3 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @clint419)


vscode-cairo/snippets/cairo.json line 226 at r4 (raw file):

    "prefix": "assert_ne!",
    "body": ["assert_ne!($1, $2);"],
    "description": "Creates an simple test"

Suggestion:

Creates an inline macro for asserting non-equality.

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @clint419)


vscode-cairo/snippets/cairo.json line 226 at r4 (raw file):

    "prefix": "assert_ne!",
    "body": ["assert_ne!($1, $2);"],
    "description": "Creates an simple test"

Maybe
Creates an assert_ne! macro.
As it should be short description.

Copy link
Contributor Author

@clint419 clint419 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)


vscode-cairo/snippets/cairo.json line 226 at r4 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Maybe
Creates an assert_ne! macro.
As it should be short description.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @clint419)

@clint419 clint419 force-pushed the add_assert_ne_macro branch from 036842b to 37de835 Compare November 20, 2023 14:57
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @clint419)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Nov 20, 2023
Merged via the queue into starkware-libs:main with commit 8699e4e Nov 20, 2023
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.

3 participants