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

chore: explicitly test all errors #76

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Conversation

jamesondh
Copy link
Contributor

@jamesondh jamesondh commented Oct 3, 2023

Motivation

Make tests more comprehensive.

Explanation of Changes

Added and modified tests to explicitly check for each error defined in each contract's error.rs. For errors that require testing using multiple contracts, the error is tested in the integration tests packages.

Failing checks are caused by an outdated rust version using a previously unstable feature. They pass with a bumped version: #77

Testing

N/A

Related PRs and Issues

Closes #58

@jamesondh jamesondh marked this pull request as ready for review October 3, 2023 22:36
@jamesondh jamesondh requested a review from a team October 3, 2023 22:37
@Thomasvdam
Copy link
Member

I think this one clashes with #78 ? Maybe we should merge that one first and then see what the conflict damage is, provided the approach in #78 is desired.

@jamesondh jamesondh force-pushed the chore/comprehensive-test-err branch 2 times, most recently from f6b546e to aa14b99 Compare October 8, 2023 17:10
@jamesondh jamesondh force-pushed the chore/comprehensive-test-err branch from aa14b99 to a036883 Compare October 9, 2023 18:47
@jamesondh
Copy link
Contributor Author

@mariocao I had to add anyhow back in this PR (despite Menna removing it as per your feedback in #78) to be able to get specific errors in the integration tests via res.unwrap_err().downcast_ref::<ContractError>(). I also tried replacing these with a #[should_panic] attribute, checking for the error enum variant, (as used in some unit tests) but the error in the integration tests only outputs the error message.

For example, in the unit tests I can use #[should_panic(expected = "IneligibleExecutor")] but in the integration tests I would have to use #[should_panic(expected = "Caller is not an eligible data request executor")], which I think is strange and inconsistent. Obviously this is a pretty minor issue but I think the anyhow dependency makes the integration tests just a bit more readable/maintainable 😅

@jamesondh jamesondh merged commit a036883 into main Oct 12, 2023
4 checks passed
@jamesondh jamesondh deleted the chore/comprehensive-test-err branch October 12, 2023 16:32
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.

🔧 Test all error conditions
3 participants