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

test(medusa): requester properties #57

Merged
merged 29 commits into from
Nov 28, 2024

Conversation

0xJabberwock
Copy link
Member

🤖 Linear

Closes GRT-XXX

@0xJabberwock 0xJabberwock self-assigned this Nov 19, 2024
@simon-something simon-something changed the base branch from dev to test/handler-cov November 21, 2024 12:08
Comment on lines 75 to 77
/// @custom:property-id 2
/// @custom:property There can only be one active request per chainId/epoch at a time
function property_onlyOneActiveRequest(uint256 _requestIdSeed) external {
Copy link
Member Author

Choose a reason for hiding this comment

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

As of now, bear in mind that prop-1 already takes into account active requests, so it asserts what prop-2 does as well – necessarily, as it is a reversion reason of the function call. Should we keep a separate prop-2 then?

Copy link
Member

Choose a reason for hiding this comment

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

no, totally fine to test multiple properties in the same function (what we did so far is then keep naming based on the first property, but could be a more generic one, like property_createRequest), have all custom natspec included and a one line comment with the id before the relevant assert

  /// @custom:property-id 1
  /// @custom:property ...
  /// @custom:property-id 2
  /// @custom:property ...
  /// @custom:property-id 3
  /// @custom:property ...
function property_test() external {
(...)
try ...
  // property 1
  assertEq...
  // property 2
...
catch 
  // property 3
  assert ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 93 to 94

assertLte(activeRequests, 1, 'prop-2: same chainId/epoch active requests');
Copy link
Member Author

Choose a reason for hiding this comment

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

Related assertions between prop-1 and prop-2 aren't exactly equal because the former updates ghost state after asserting, and the latter counts on already updated ghost state. So, in presence of and within try blocks, should the ghost state be updated before or after asserting?

Copy link
Member

Choose a reason for hiding this comment

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

that's why having explicit Hoare logic would help I think (I mentioned it somewhere, but too late to refactor it everywhere, perhaps) -> the assertion(s) is/are the post-condition(s), ghost is just an extra-help to track state between calls (ie "propagate" the state change of the assertion, in other word, they are the pre-conditions of another test), imo, post-condition shouldn't rely on having the statement updating a ghost variable.

short version: after

@0xJabberwock 0xJabberwock changed the title test(medusa): ebo request creator properties test(medusa): requester properties Nov 28, 2024
@simon-something simon-something marked this pull request as ready for review November 28, 2024 11:12
@simon-something simon-something merged commit 9c8dce2 into test/handler-cov Nov 28, 2024
7 checks passed
@simon-something simon-something deleted the test/property-ebo-request-creator branch November 28, 2024 11:12
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.

2 participants