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

Fix CI workflow and examples tests #1228

Merged
merged 14 commits into from
Aug 1, 2024
Merged

Conversation

renzobanegass
Copy link
Contributor

@renzobanegass renzobanegass commented Jul 31, 2024

Summary

Issue #1190

This PR addresses the following issues:

  • Ensures examples .wasm files are built before running tests by updating CONTRIBUTING.md.
  • Modifies CI workflow to build and test all examples in exception of factory-contracts.
  • Adds missing unit-testing feature for examples/non-fungible-token and examples/versioned.
  • Refactors deprecated features in examples.

Changes

  • Updated CONTRIBUTING.md to include instructions for building examples.
  • Modified .github/workflows/test_examples_small.yml and .github/workflows/test_examples.yml to build and test more examples.
  • Added unit-testing feature for examples/non-fungible-token and examples/versioned in their Cargo.toml.
  • Replaced deprecated UnorderedMap with LookupMap in examples/versioned.

Testing

All tests have been run locally and pass successfully. In exception for the factory-contract tests. Also the CI has been modified and we need to check which errors come up when the pipeline runs.

LookupMap

Regarding this change I found that LookupMap is a suitable replacement for UnorderedMap based on the NEAR SDK documentation and the NEAR collections overview.

Key Points from Documentation:

LookupMap Features:

  • LookupMap provides efficient key-value storage with O(1) complexity for access, insertion, and deletion operations.
  • It is a good alternative for use cases where the order of elements does not matter.

UnorderedMap Deprecation:

  • UnorderedMap is deprecated, meaning it's no longer recommended for use in new code.
  • The NEAR SDK documentation and source code changes have suggested moving towards other data structures like LookupMap or TreeMap for similar use cases.

@renzobanegass renzobanegass marked this pull request as draft July 31, 2024 22:50
@renzobanegass
Copy link
Contributor Author

I fixed some typos and errors I introduced in the yaml files, now the pipeline fails as expected on the factory-contract, I'll check later when test small examples finishes if that's still working properly as I didn't really modify it, I reverted the changes I made.

As for the factory-contract error I really don't know much about this so I would appreciate any help.

@renzobanegass
Copy link
Contributor Author

Ok, I found a workaround by running status-message before factory-contract as it seems it needs it to be built before. Now the tests are running but there seems to be an error in the non-fungible-token regarding an unused trait, but this already seems a bit out of scope in regards to the original issue. I wait for any feedback on how this should be handled.

@frol
Copy link
Collaborator

frol commented Aug 1, 2024

@renzobanegass Add pub in front of trait ValueReturnTrait in examples/non-fungible-token/test-approval-receiver/src/lib.rs line 18, this way it will communicate to Rust compiler that it is going to be used publically and won't mark it as "not used".

@renzobanegass
Copy link
Contributor Author

renzobanegass commented Aug 1, 2024

@renzobanegass Add pub in front of trait ValueReturnTrait in examples/non-fungible-token/test-approval-receiver/src/lib.rs line 18, this way it will communicate to Rust compiler that it is going to be used publically and won't mark it as "not used".

I also added it to the examples/non-fungible-token/test-token-receiver/src-lib.rs:18 as it had the same warning when running tests locally.

@renzobanegass
Copy link
Contributor Author

Just in case it helps with this, I added some logging to the failing examples/factory-contract/tests/workspaces.rs test and it throws this error regarding an Account that doesnt exist

ExecutionOutcome { transaction_hash: 9fjyUL5SkSqJFkaJwJNppiyk3DdQMnKN7LcZ3ZyTaE6s, block_hash: 3jmU8jDcEcJHERGBNMaSB43Xn6D2yD5kfUPz5RsKE5iy, logs: [], receipt_ids: [Ea5yKjwAAbfFnVp73QYVQtVTdX6TfWVszDnztrtBMCUW], gas_burnt: NearGas { inner: 888137757690 }, tokens_burnt: NearToken { inner: 88813775769000000000 }, executor_id: AccountId("status-top-level-account-long-name"), status: Failure(ActionError(ActionError { index: Some(0), kind: AccountDoesNotExist { account_id: AccountId("status-top-level-account-long-name") } })) },

@frol
Copy link
Collaborator

frol commented Aug 1, 2024

The NEAR SDK documentation and source code changes have suggested moving towards other data structures like LookupMap or TreeMap for similar use cases.

Could you, please, contribute to those places to recommend IterableMap instead? (It got available with the most recent release, which is why it is still not widely recommended - we did not catch all the places)

@renzobanegass
Copy link
Contributor Author

The NEAR SDK documentation and source code changes have suggested moving towards other data structures like LookupMap or TreeMap for similar use cases.

Could you, please, contribute to those places to recommend IterableMap instead? (It got available with the most recent release, which is why it is still not widely recommended - we did not catch all the places)

Should I change the LookupMap to an IterableMap instead?

@frol
Copy link
Collaborator

frol commented Aug 1, 2024

Should I change the LookupMap to an IterableMap instead?

Yes, please

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@renzobanegass I'd like to also invite you to join @race-of-sloths. Just mention the bot in a GitHub comment. Here is how it looks: #1230 (comment)

@frol frol marked this pull request as ready for review August 1, 2024 15:22
@frol frol merged commit f0b522f into near:master Aug 1, 2024
29 checks passed
@frol frol mentioned this pull request Aug 1, 2024
@renzobanegass
Copy link
Contributor Author

@race-of-sloths include this PR please

@race-of-sloths
Copy link

race-of-sloths commented Aug 1, 2024

@renzobanegass Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
New Sloth joined the Race! Welcome!

Shows profile picture for the author of the PR

Current status: executed
Reviewer Score
@frol 3

Your contribution is much appreciated with a final score of 3!
You have received 40 (30 base + 10 weekly bonus) Sloth points for this contribution

Another weekly streak completed, well done @renzobanegass! To keep your weekly streak and get another bonus make pull request next week! Looking forward to see you in race-of-sloths

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

  • Tag @race-of-sloths inside your pull requests
  • Wait for the maintainer to review and score your pull request
  • Check out your position in the Leaderboard
  • Keep weekly and monthly streaks to reach higher positions
  • Boast your contributions with a dynamic picture of your Profile

For maintainers:

  • Score pull requests that participate in the Race of Sloths
  • Engage contributors with fair scoring and fast responses so they keep their streaks
  • Promote the Race to the point where the Race starts promoting you
  • Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands
  • For contributors
    • Include a PR: @race-of-sloths include to enter the Race with your PR
  • For maintainers:
    • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
    • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
    • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

@race-of-sloths
Copy link

🔄 The PR has been merged

@frol, please score the PR with @race-of-sloths score [1/2/3/5/8/13]. The contributor deserved it
As an alternative option auto-scoring [1, 2] will be applied to this PR!

@frol
Copy link
Collaborator

frol commented Aug 1, 2024

@race-of-sloths score 3

@race-of-sloths
Copy link

🌟 Score recorded!

@frol, thank you for scoring this pull request in the Race of Sloths!

@renzobanegass renzobanegass deleted the fix-ci-workflow branch August 2, 2024 18:56
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