Repository navigation
test: fix contract integration tests - #1725
Conversation
Add unit tests for `validate_tee()` and integration test for expired attestation resharing Fixes #1144
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive test coverage for TEE attestation validation and expired attestation resharing functionality. The changes address issue #1144 by adding unit tests for the validate_tee() function and a full integration test for the expired attestation resharing flow.
Key Changes:
- Added 5 unit tests covering different scenarios of
validate_tee()including valid attestations, missing attestations, expired attestations, and invalid attestations - Added integration test verifying the complete flow of detecting expired attestations and triggering resharing
- Refactored existing Rust test to focus specifically on verifying resharing trigger rather than complete resharing flow
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pytest/tests/test_expired_attestation_resharing.py |
New integration test that verifies verify_tee() correctly triggers resharing when a participant's attestation expires, and validates the participant is removed from the active set |
pytest/common_lib/contracts.py |
Added two new contract method enums (SUBMIT_PARTICIPANT_INFO and VERIFY_TEE) to support the integration test |
crates/contract/tests/inprocess/attestation_submission.rs |
Refactored test to focus on verifying resharing state transition on expired attestation, removed unused import (KeyEventId), and updated documentation to clarify the test's purpose |
crates/contract/src/tee/tee_state.rs |
Added 5 comprehensive unit tests for validate_tee() covering all validation scenarios: all valid, missing attestation, expired attestation, not yet expired, and invalid attestation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gilcu3
left a comment
There was a problem hiding this comment.
This PR does not seem to solve the issue at hand. It is adding a system test (pytest), while the issue mentions integration test (sandbox tests)
I assumed the integration tests were the ones in the Lines 24 to 27 in ac75f07 Looks like the naming we use for different test types is still a bit confusing. No worries though — I can implement a sandbox test instead. Thanks for catching that! |
Yes, this used to be a contention, but we aligned on the respective definitions in August thanks to @DSharifi's initiative. edit: tracking issue: #1732 |
|
I’ve reimplemented the pytest as an integration test. Feel free to re-review, @gilcu3 @kevindeforth. |
There was a problem hiding this comment.
LGTM. Only blocked is that a function should be converted to unit test, or not simplified if it should remain an integration test, as per the issue. See #1725 (comment)
kevindeforth
left a comment
There was a problem hiding this comment.
Hard blocker right now are the expiry times - we shouldn't need any "margin" there.
Soft blocker is added checks in unit tests, ensuring the resulting state corresponds to the expected state. Comparing number of participants and account id is not enough here, as the resharing does not go through a voting.
We expect all participant details of the remaining participants to match. And we expect some sensible limits for the threshold.
@kevindeforth could you take a look at whether the asserts look better now, and whether there are any other asserts you’d add? Also, @gilcu3, feel free to re-review. Thanks! |
gilcu3
left a comment
There was a problem hiding this comment.
Still looks good to me, left a few (optional) comments
kevindeforth
left a comment
There was a problem hiding this comment.
Thank you! Only nits.
Add unit tests for
validate_tee()and integration test for expired attestation resharing.Fixes #1144