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

Extract Semantics Tests #4223

Closed
ekpyron opened this issue Jun 4, 2018 · 9 comments
Closed

Extract Semantics Tests #4223

ekpyron opened this issue Jun 4, 2018 · 9 comments
Assignees

Comments

@ekpyron
Copy link
Member

ekpyron commented Jun 4, 2018

Part of #3486 concerning semantics tests (i.e. "end-to-end"-tests).

Similarly to #3644 the semantics tests (resp. end-to-end tests) should be extracted into individual files. ìsoltest should support running the semantics tests interactively.

This issue is meant to discuss the syntax of the test expectations and the encoding of arguments and expected results.

@ekpyron ekpyron self-assigned this Jun 4, 2018
@ekpyron
Copy link
Member Author

ekpyron commented Jun 4, 2018

Some initial and unrefined ideas for the expectation syntax:

The syntax originally suggested in #3486, slightly adapted to be closer to the syntax already used for the syntax tests, would be the following:

contract test {
   ...
}
// ----
// f(uint,bytes32): 0x123000, 456 -> 123, true
// g(string): "abc" -> X

We could also consider the following to keep arguments and their values closer together and adopt a more "function-call-like" syntax. It might also make sense to explicitly state the types of return values:

contract test {
   ...
}
// ----
// f(uint(0x123000),bytes32(456)): uint(123), bool(true)
// g(string("abc")): REVERT

The signature can easily be extracted during parsing of the expectations.

The colon could be also be replaced by "==" and the return arguments given as tuple, e.g.
// f(uint(0x123000),bytes32(456)) == (uint(123), bool(true))

The parser for the return values should remember the used formatting and recreate expectations with the same format when updating expectations (for isoltest).

The first line of the expectations could optionally be an explicit deploy command:

contract test {
  constructor(uint256, bool) {}
   ...
}
// ----
// DEPLOY: test(uint256(0x123),bool(false))
// f(uint(0x123000),bytes32(456)) == (uint(123), bool(true))
// g(string("abc")): REVERT

Maybe a simple constructor call would suffice, however that may be more ambiguous:
// test(uint256(0x123),bool(false))

Additionally there needs to be an optional way to send ether to both the constructor and functions. As a suggestion:

contract test {
  constructor(uint256, bool) {}
   ...
}
// ----
// DEPLOY: test(uint256(0x123),bool(false)) <- 20
// f(uint(0x123000),bytes32(456)) <- 40 == uint(123), bool(true)
// g(string("abc")): REVERT

I'll start writing the general framework already and I'll keep thinking about further syntax options for the expectations in the process - suggestions are welcome.

@chriseth
Copy link
Contributor

chriseth commented Jun 4, 2018

To be honest, I find f(uint(0x123000),bytes32(456)) rather difficult to read and also correlate with what function is called on the contract.

Perhaps we should split inputs and expectations across two lines. Ether could be given in square brackets. Also I'm not sure if we should implement a full ABI-encoder here. If we always assume expectations to be abi-encoded, we lose the ability to test data encoded invalidly and also providing input data that is encoded in a weird way.

// DEPLOY: test[20]: 0x123, false
// f(uint256,bytes32)[40]: 0x123000, 456
// -> 123, true
// g(string): 0x20, 3, "abc"
// REVERT

If we assume each field to be exactly 32 bytes with decimals padded on the left and hex and strings padded on the right, then we also need a way to specify things that are not padded, but that could be left for the tests that do not work like that.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 7, 2018

OK, I adopted that syntax.

Regarding padding: the current WIP PR pads both decimals and hex numbers on the left, but hex strings hex"ABCD" are padded on the right - I think that's clearer than 1 and 0x1 resulting in different padding.
I also added dedicated syntax to encode unpadded bytes rawbytes(0x12, 0x00, 0xAB) and dynamic length strings string("abc") (i.e. 0x20, 3, "abc").

Should expectation and result mismatch, the PR attempts to format the actual result exactly like the expected result and, if that's impossible, falls back to hex numbers (and unpadded bytes in the absence of 32-byte alignment).

(the PR still needs quite some cleaning up in detail, though)

@chriseth
Copy link
Contributor

chriseth commented Jun 7, 2018

The padding is probably a good idea, yes!

What is the difference between hex"1200ab" and rawbytes(0x12, 0x00, 0xab)? Isn't the first format better?

About string("abc"): This only works if the string is the only argument or return value. I know we have the helper also in the C++ code, but I don't know if it is more confusing than helpful.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 7, 2018

OK - I can remove the string helper - you may be right and always using the encoding explicitly may be less confusing.

The difference between hex"1200ab" and rawbytes(0x12, 0x00, 0xab) is that the former is padded to 32-bytes (to the right), whereas the latter is unpadded. rawbytes should only ever be needed, if something is wrong, i.e. if we want to supply non-32-byte-aligned input or if the output is not aligned. Maybe I should rename it to unpaddedbytes, though...

@ekpyron
Copy link
Member Author

ekpyron commented Jun 7, 2018

One issue is that the RPC interface doesn't distinguish between revert and no return data, so I'm not sure whether we should just use -> (empty return) instead of REVERT, or whether we should just let every function that is called in the tests return something, if only a bool true...

@chriseth
Copy link
Contributor

chriseth commented Jun 8, 2018

The "success condition" should be part of the receipt. It might be that cpp-ethereum does not implement it or the json-rpc parser in the testing framework does not implement it.

@ekpyron
Copy link
Member Author

ekpyron commented Jun 8, 2018

So far I haven't found a way to properly deduce a revert - probably related: ethereum/aleth#4157

@Marenz
Copy link
Contributor

Marenz commented Jan 27, 2020

This issue existed to track the progress of isoltest in regards with semantic test capabilities. @erak says this is done now and we favor closing this issue and create a new one that is purely for extracting more semantic tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants