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

ci: add CI script for pop-api tests and integration tests #228

Merged
merged 18 commits into from
Aug 26, 2024

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Aug 22, 2024

Link to issue: #109

  • Build contracts listed under integration-tests/contracts when running cargo build --features integration-tests
  • CI to run integration tests of contracts

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.10%. Comparing base (6482a43) to head (ba66dd9).

@@             Coverage Diff              @@
##           daan/api     #228      +/-   ##
============================================
+ Coverage     34.08%   34.10%   +0.02%     
============================================
  Files            34       34              
  Lines          3013     3011       -2     
  Branches       3013     3011       -2     
============================================
  Hits           1027     1027              
+ Misses         1954     1952       -2     
  Partials         32       32              

see 2 files with indirect coverage changes

@evilrobot-01
Copy link
Collaborator

Hmmm, could we not have solved this with a build.rs that takes a dependency on cargo-contract to build the relevant contracts?

@chungquantin
Copy link
Collaborator Author

@evilrobot-01 Thanks for the feedback. I have redesigned the CI to simplify the script and move the contract build process to the integration-tests crate build script. Please take a look and let me know if there are any needed changes.

cc: @Daanvdplas

@chungquantin chungquantin changed the title ci: add CI script for contract integration tests ci: add CI script for api tests and integration tests Aug 22, 2024
@chungquantin chungquantin changed the title ci: add CI script for api tests and integration tests ci: add CI script for pop-api tests and integration tests Aug 22, 2024
@chungquantin chungquantin added the api Pop API label Aug 22, 2024
@evilrobot-01
Copy link
Collaborator

Gave it a quick test and it failed as I did not specify the 'contracts' feature. Is there any reason that this needs to be specified explicitly? The goal is that we can just run cargo test in the integration tests folder and the build script will ensure that the contracts are built, as the integration tests are reliant on them being built.

Running it again with the feature then worked, but it took a long time on 'building' after having cleared out the caches of all contracts and integration tests. Would outputting something to the console help here possibly? Just a message stating 'building xxx contract...' for each should be enough.

@peterwht
Copy link
Collaborator

Great job, this will be quite useful! It would be a nice addition to have all .wasms get outputted to an artifacts folder. That way we can persist the same artifacts without requiring a rebuild of the contracts in CI.

@chungquantin
Copy link
Collaborator Author

@peterwht Contracts are archived as artifacts
@evilrobot-01 Remove contract feature, contracts are built whenever we run cargo test in the integration-tests folder

@evilrobot-01
Copy link
Collaborator

Rebuilding contracts each time should be instant if no changes and ensures that any local contract changes are always included when running tests, improving our devex flow.

In the past we had to know to run all these extra commands, now we just make changes and run the tests and everything 'just works'

Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

Thanks a lot Tin! Provided a suggested refactor.

pop-api/integration-tests/build.rs Outdated Show resolved Hide resolved
Co-authored-by: Daan van der Plas <[email protected]>
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Looks fine and works as is. I personally dont see the need for storing the artifacts to would be interested to see how they will actually be used.

I still think that a console log per contract build would be better than nothing whatsoever, especially as we expect many more test contracts to be added in the near term.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
pop-api/integration-tests/build.rs Outdated Show resolved Hide resolved
@chungquantin
Copy link
Collaborator Author

Update:

  • Remove archiving artifacts in the CI script
  • Build mode is changed to Debug mode instead so we can track the debug message of the ink! contract.
  • Build artifact is added with CodeOnly mode because we only need .wasm file for the integration tests, no metadata needed.

cc: @evilrobot-01 @peterwht @Daanvdplas

Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

LGTM!

@chungquantin chungquantin merged commit 1b78c11 into daan/api Aug 26, 2024
10 checks passed
@chungquantin chungquantin deleted the chungquantin/ci-contract_integration_tests branch August 26, 2024 13:04
chungquantin added a commit that referenced this pull request Sep 6, 2024
chungquantin added a commit that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants