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

Addition of End-to-End Tests for Evolution Flow #138

Merged
merged 15 commits into from
Nov 6, 2023

Conversation

magecnion
Copy link
Contributor

@magecnion magecnion commented Nov 3, 2023

PR Type:

Tests


PR Description:

This PR introduces end-to-end tests for the evolution flow. It includes refactoring of the test-create-collection file and addition of the test-evolution file. The PR also modifies the Solidity selector of the EvolvedWithExternalURI log and updates the test to reflect this change. Additionally, it includes updates to the configuration file for the tests.


PR Main Files Walkthrough:

files:

ownership-chain/precompile/laos-evolution/src/lib.rs: The Solidity selector of the EvolvedWithExternalURI log has been updated.
ownership-chain/precompile/laos-evolution/src/tests.rs: The test for the selector of the EvolvedWithExternalURI log has been updated to match the new selector.
ownership-chain/e2e-tests/tests/config.ts: The configuration file for the tests has been updated with new constants related to the LAOS Evolution Contract and the chain configuration.
ownership-chain/e2e-tests/tests/test-create-collection.ts: The test-create-collection file has been refactored to improve the structure and readability of the tests.
ownership-chain/e2e-tests/tests/test-evolution.ts: A new file has been added to test the evolution flow. It includes tests for minting and evolving assets.
ownership-chain/e2e-tests/tests/util.ts: A new function has been added to convert a slot and owner to a token ID.

@magecnion magecnion linked an issue Nov 3, 2023 that may be closed by this pull request
@magecnion magecnion marked this pull request as ready for review November 3, 2023 15:30
@magecnion
Copy link
Contributor Author

/describe

@magecnion
Copy link
Contributor Author

/review

@github-actions github-actions bot changed the title Feature add evolution e2e test Addition of End-to-End Tests for Evolution Flow Nov 3, 2023
Copy link

github-actions bot commented Nov 3, 2023

PR Analysis

  • 🎯 Main theme: Addition of end-to-end tests for the evolution flow in the Laos ownership chain.
  • 📝 PR summary: This PR includes changes to the Laos ownership chain, specifically the addition of end-to-end tests for the evolution flow. It also includes some refactoring of existing tests and code, and fixes to selectors in the Laos Evolution Contract.
  • 📌 Type of PR: Tests
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 3
    This PR includes a significant amount of new test code, which requires careful review to ensure correctness and coverage. However, the changes to existing code are relatively minor, mostly involving refactoring and selector updates.
  • 🔒 Security concerns: No
    The PR primarily involves test code and minor refactoring, and does not appear to introduce any new security concerns.

PR Feedback

  • 💡 General suggestions: The PR is generally well-structured and the addition of end-to-end tests is a good practice. However, it would be beneficial to include more detailed commit messages and PR descriptions to provide context for the changes. Also, consider splitting large PRs into smaller ones to make the review process more manageable.

  • 🤖 Code feedback:

    • relevant file: ownership-chain/e2e-tests/tests/test-evolution.ts
      suggestion: Consider adding more edge case tests for the evolution flow. For example, what happens if the asset is evolved with an invalid token URI? [medium]
      relevant line: "+ step("when asset is evolved it should change token uri", async function () {"

    • relevant file: ownership-chain/e2e-tests/tests/test-evolution.ts
      suggestion: It would be beneficial to add assertions after each test step to ensure that the state of the system is as expected. This can help catch issues early if a test fails. [medium]
      relevant line: "+ step("when asset is minted it should return token uri", async function () {"

    • relevant file: ownership-chain/e2e-tests/tests/test-create-collection.ts
      suggestion: Consider adding tests to check the state of the system after a collection is created. For example, you could check that the collection count has increased by 1. [medium]
      relevant line: "+ step("when collection is created, it should return owner", async function () {"

    • relevant file: ownership-chain/e2e-tests/tests/util.ts
      suggestion: It would be beneficial to add error handling in the 'slotAndOwnerToTokenId' function. If the function returns null, this could lead to issues in the tests that use this function. [important]
      relevant line: "+export function slotAndOwnerToTokenId(slot: string, owner: string): string | null {"

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

ghost
ghost previously approved these changes Nov 6, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

some nitpicks

ownership-chain/e2e-tests/tests/config.ts Outdated Show resolved Hide resolved
});

step("when collection does not exist owner of call should fail", async function () {
const collectionId = "0";
try {
await contract.methods.ownerOfCollection(collectionId).call();
expect.fail("Expected error was not thrown"); // Ensure an error is thrown
} catch (error) {
expect(error.message).to.be.eq(
"Returned error: VM Exception while processing transaction: revert"
Copy link

Choose a reason for hiding this comment

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

this is a generic error message, is there any way to expect more specific message?

Copy link
Contributor Author

@magecnion magecnion Nov 6, 2023

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

could this be a solution? https://ethereum.stackexchange.com/a/84551

ownership-chain/e2e-tests/tests/test-evolution.ts Outdated Show resolved Hide resolved
ownership-chain/e2e-tests/tests/test-create-collection.ts Outdated Show resolved Hide resolved
ownership-chain/e2e-tests/tests/test-evolution.ts Outdated Show resolved Hide resolved
ownership-chain/e2e-tests/tests/util.ts Outdated Show resolved Hide resolved
ownership-chain/e2e-tests/tests/util.ts Outdated Show resolved Hide resolved
ownership-chain/e2e-tests/tests/util.ts Outdated Show resolved Hide resolved
@magecnion magecnion merged commit 6c686c4 into main Nov 6, 2023
7 checks passed
@magecnion magecnion deleted the feature/add_evolution_e2e_test branch November 6, 2023 12:52
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.

EVM events can be obtained via RPC-EVM! (integration test?)
2 participants