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

test: Add unit tests to artifacts.rs module #2680

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Abeeujah
Copy link

@Abeeujah Abeeujah commented Nov 16, 2024

Closes #2625

Introduced changes

Tests directory in crates/scarb-api/tests/data/basic_package for integration tests starknet artifacts.

Integration tests artifacts only get built when there's a tests directory in a cairo package

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

Good work so far. The test_unique_artifacts looks good, left some suggestions for improving test_load_contracts_artifacts

crates/scarb-api/src/artifacts.rs Outdated Show resolved Hide resolved
crates/scarb-api/src/artifacts.rs Outdated Show resolved Hide resolved
crates/scarb-api/src/artifacts.rs Outdated Show resolved Hide resolved
@cptartur cptartur self-requested a review November 22, 2024 13:23
Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

The code looks good 👌

Let's also change this line in ci.yml

cargo test --package scarb-api --features scarb_2_8_3 get_starknet_artifacts_path

to

cargo test --package scarb-api --features scarb_2_8_3

so all these tests run in CI.

@Abeeujah
Copy link
Author

Abeeujah commented Dec 4, 2024

Hi @cptartur This particular test keeps failing, tests::package_matches_version_requirement_test

@Abeeujah Abeeujah requested a review from cptartur December 4, 2024 23:57
@cptartur
Copy link
Member

cptartur commented Dec 5, 2024

Hi @cptartur This particular test keeps failing, tests::package_matches_version_requirement_test

Rerunning tests, this shouldn't fail and there's nothing in your code that should cause that.

@Abeeujah
Copy link
Author

Abeeujah commented Dec 5, 2024

Okay, thank you @cptartur you've been great all through. ✊

Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

@Abeeujah verified the failing tests:

test_load_contracts_artifacts is failing because there's no integrationtest target in basic_package. Please add basic_package/tests directory and add any test file with any cairo file with a test in there (can even be something like assert 2 == 2) and it should resolve itself.

The other failing test, see my comment

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Abeeujah
Copy link
Author

Abeeujah commented Dec 5, 2024

When I did, a couple previous tests were failing, so I resorted to generating the file using indoc.

failures:

---- tests::get_starknet_artifacts_path_for_test_build_when_integration_tests_exist stdout ----
thread 'tests::get_starknet_artifacts_path_for_test_build_when_integration_tests_exist' panicked at crates/scarb-api/src/lib.rs:325:36:
called `Result::unwrap()` on an `Err` value: Os { code: 17, kind: AlreadyExists, message: "File exists" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::package_matches_version_requirement_test stdout ----
thread 'tests::package_matches_version_requirement_test' panicked at crates/scarb-api/src/lib.rs:421:9:
assertion failed: !package_matches_version_requirement(&scarb_metadata, "starknet",
            &VersionReq::parse("2.8").unwrap()).unwrap()

---- tests::get_starknet_artifacts_path_for_test_build stdout ----
thread 'tests::get_starknet_artifacts_path_for_test_build' panicked at crates/scarb-api/src/lib.rs:307:9:
assertion `left == right` failed
  left: StarknetArtifactsFiles { base: "/tmp/.tmp3EXCX4/target/dev/basic_package_integrationtest.test.starknet_artifacts.json", other: ["/tmp/.tmp3EXCX4/target/dev/basic_package_unittest.test.starknet_artifacts.json"] }
 right: StarknetArtifactsFiles { base: "/tmp/.tmp3EXCX4/target/dev/basic_package_unittest.test.starknet_artifacts.json", other: [] }


failures:
    tests::get_starknet_artifacts_path_for_test_build
    tests::get_starknet_artifacts_path_for_test_build_when_integration_tests_exist
    tests::package_matches_version_requirement_test

test result: FAILED. 10 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 21.40s

These were the failing tests.

So I adjusted test_load_contracts_artifacts to this

#[test]
    #[cfg_attr(not(feature = "scarb_2_8_3"), ignore)]
    fn test_load_contracts_artifacts() {
        let temp = crate::tests::setup_package("basic_package");
        let tests_dir = temp.join("tests");
        fs::create_dir(&tests_dir).unwrap();

        temp.child(tests_dir.join("test.cairo"))
            .write_str(indoc!(
                r"
                #[test]
                fn mock_test() {
                    assert!(true);
                }
            "
            ))
            .unwrap();

        ScarbCommand::new_with_stdio()
            .current_dir(temp.path())
            .arg("build")
            .arg("--test")
            .run()
            .unwrap();

        // Define path to the generated artifacts
        let base_artifacts_path = temp.to_path_buf().join("target").join("dev");

        // Get the base artifact
        let base_file = Utf8PathBuf::from_path_buf(
            base_artifacts_path.join("basic_package_integrationtest.test.starknet_artifacts.json"),
        )
        .unwrap();

        // Load other artifact files and add them to the temporary directory
        let other_files = vec![Utf8PathBuf::from_path_buf(
            base_artifacts_path.join("basic_package_unittest.test.starknet_artifacts.json"),
        )
        .unwrap()];

        // Create `StarknetArtifactsFiles`
        let artifacts_files = StarknetArtifactsFiles::new(base_file, other_files);

        // Load the contracts
        let result = artifacts_files.load_contracts_artifacts();
        println!("{result:?}");

        // Ensure no errors and non-empty result
        assert!(result.is_ok());

        // Assert the Contract Artifacts are loaded.
        let artifacts_map = result.unwrap();
        assert!(!artifacts_map.is_empty());
        assert!(artifacts_map.contains_key("ERC20"));
        assert!(artifacts_map.contains_key("HelloStarknet"));
    }
    ```
    
    And sorry I forgot I didn't push that change... Just Did
    
![Screenshot_20241205_145214](https://github.com/user-attachments/assets/f0d55acd-38f6-4252-964d-82e5ad47c55d)

@Abeeujah Abeeujah requested a review from cptartur December 5, 2024 13:53
Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

Looks good, just address the suggestions regarding ci.yml and we should be good to merge

@@ -141,7 +141,7 @@ jobs:
- run: |
cargo test --package forge --features scarb_2_8_3 e2e::features
- run: |
cargo test --package scarb-api --features scarb_2_8_3 get_starknet_artifacts_path
cargo test --package scarb-api --features scarb_2_8_3
Copy link
Member

Choose a reason for hiding this comment

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

Let's replace

      - run: |
          cargo test --package scarb-api --features scarb_2_8_3

with

      - run: |
          cargo test --package scarb-api --features scarb_2_8_3 get_starknet_artifacts_path
      - run: |
          cargo test --package scarb-api --features scarb_2_8_3 test_load_contracts_artifacts

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the line // TODO(#2625) add unit tests from this file

@Abeeujah
Copy link
Author

Done @cptartur

@cptartur
Copy link
Member

@Abeeujah seems that lint and fmt failed, please run ./scripts/scarbfmt.sh to fix format and run cargo lint and address any issues that it throws out.

Also it seems that the file you added to basic_package broke get_starknet_artifacts_path_for_test_build and get_starknet_artifacts_path_for_test_build_when_integration_tests_exist. I'd remove the file and use the original approach you did with .write_str to a created file to create test.

Comment on lines +159 to +188
ScarbCommand::new_with_stdio()
.current_dir(temp.path())
.arg("build")
.arg("--test")
.run()
.unwrap();

// Define path to the generated artifacts
let base_artifacts_path = temp.to_path_buf().join("target").join("dev");

// Get the base artifact
let base_file = Utf8PathBuf::from_path_buf(
base_artifacts_path.join("basic_package_integrationtest.test.starknet_artifacts.json"),
)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the logic that creates the test file in basic_package that was added at some point.

Copy link
Author

Choose a reason for hiding this comment

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

Done @cptartur

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.

Add unit tests to artifacts.rs module
2 participants