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

feat(test): adding JS tests to the CI #284

Merged
merged 46 commits into from
Oct 3, 2024
Merged

feat(test): adding JS tests to the CI #284

merged 46 commits into from
Oct 3, 2024

Conversation

Mohiiit
Copy link
Contributor

@Mohiiit Mohiiit commented Sep 25, 2024

Adding starknet js tests

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Testing

What is the current behavior?

Resolves: #NA

What is the new behavior?

  • Ideally js tests should run in CI to check nothing is broken

Does this introduce a breaking change?

No

@antiyro antiyro added the testing Related to tests and test infrastructure label Sep 25, 2024
@antiyro antiyro changed the title Test/js test feat(test): adding JS tests to the CI Sep 25, 2024
@Mohiiit Mohiiit marked this pull request as ready for review September 25, 2024 18:49
Copy link
Contributor

@apoorvsadana apoorvsadana left a comment

Choose a reason for hiding this comment

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

small review :)

path: cairo/target
key: ${{ runner.os }}-cairo-${{ github.sha }}
fail-on-cache-miss: true
- name: Setup and run dev chain
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Setup and run dev chain
- name: Setup dev chain and run tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

await provider.waitForTransaction(declareResponse.transaction_hash);

// Add assertion to check if the class hash is valid
expect(declareResponse.class_hash).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

we should get class hash and see if it's on chain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

);

// Add assertion to check if the contract address is valid
expect(deployResult.contract_address[0]).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

we can maybe call get class hast at and check if it's registered on chain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

);

// Add assertion to check if the account address is valid
expect(deployResult.contract_address[0]).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

sudo apt-get update
sudo apt-get install -y clang llvm libudev-dev protobuf-compiler
- uses: rui314/setup-mold@v1
- uses: software-mansion/setup-scarb@v1
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you are calling 2 times software-mansion/setup-scarb@v1 for version 2.8.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, removed

- uses: foundry-rs/foundry-toolchain@v1
with:
version: nightly
- uses: software-mansion/setup-scarb@v1
Copy link
Member

Choose a reason for hiding this comment

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

also here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

uses: actions/cache@v3
with:
path: target/release/madara
key: ${{ runner.os }}-madara-${{ github.sha }}
Copy link
Member

Choose a reason for hiding this comment

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

Here it appears that you are generating you key over the commit hash this doesnt invalidate the cache on each commit?

Copy link
Member

Choose a reason for hiding this comment

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

I have checked maybe you can use:

key: ${{ runner.os }}-madara-${{ hashFiles('Cargo.lock') }}

to generate a key based on the lock changes or take in account even more files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

js_test:
name: Run JS Tests
uses: ./.github/workflows/starknet-js-test.yml
needs: build
Copy link
Member

Choose a reason for hiding this comment

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

why does js_test depends on build? could we parallelize that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need a build of madara to have it's instance running before running tests on it

so the flow is that we run madara and we run tests on it, so for that we need the latest build

Copy link
Member

Choose a reason for hiding this comment

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

okk nice

Copy link
Member

Choose a reason for hiding this comment

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

could we move the js_tests unde tests/js_tests?

Copy link
Contributor Author

@Mohiiit Mohiiit Sep 27, 2024

Choose a reason for hiding this comment

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

you mean inside the crates?

Copy link
Member

Choose a reason for hiding this comment

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

no in the root folder directly, so we can add more tests in the futur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

Copy link
Member

@antiyro antiyro left a comment

Choose a reason for hiding this comment

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

lgtm

@antiyro antiyro merged commit 22f8343 into main Oct 3, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to tests and test infrastructure
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants