-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Extract semantic tests Pt. 1 #5736
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
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5736 +/- ##
===========================================
- Coverage 88.35% 88.13% -0.23%
===========================================
Files 348 352 +4
Lines 33431 33600 +169
Branches 4005 4045 +40
===========================================
+ Hits 29538 29612 +74
- Misses 2535 2602 +67
- Partials 1358 1386 +28
|
test/InteractiveTests.h
Outdated
{"JSON AST", "libsolidity", "ASTJSON", false, false, &ASTJSONTest::create}, | ||
{"SMT Checker", "libsolidity", "smtCheckerTests", true, false, &SyntaxTest::create}, | ||
{"SMT Checker JSON", "libsolidity", "smtCheckerTestsJSON", true, false, &SMTCheckerTest::create} | ||
// {"Yul Optimizer", "libyul", "yulOptimizerTests", false, false, &yul::test::YulOptimizerTest::create}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not stay in the final PR.
test/libsolidity/SemanticTest.cpp
Outdated
if(_actualResults && _formatted) | ||
_stream << endl; | ||
|
||
// string result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not stay in the final PR
namespace test | ||
{ | ||
|
||
class ExpectationParser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document.
test/libsolidity/ExpectationParser.h
Outdated
class ExpectationParser | ||
{ | ||
public: | ||
struct FunctionCallResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
to the next line
bytes byteRange(it, _bytes.end()); | ||
stringstream resultStream; | ||
// TODO: Convert from compact big endian if padded | ||
resultStream << fromBigEndian<u256>(byteRange); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could directly take _bytes
} | ||
} | ||
|
||
string ExpectationParser::bytesToString(bytes const& _bytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are decimal integers involved here, so I think the name should be different :)
There was an error when running
Please check that your changes are working as intended. |
e93237d
to
34a8cfc
Compare
49ee8a4
to
b7558a4
Compare
@@ -6,6 +6,7 @@ Language Features: | |||
|
|||
Compiler Features: | |||
* Control Flow Graph: Warn about unreachable code. | |||
* Tests: Add infrastructure to ``isoltest`` for running semantic tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to move that to "build system" or rename "Build system" to "infrastructure" or something like that. At least it is nothing that concerns the user.
// f(uint256): 1 | ||
// -> 2 | ||
// g(uint256, uint256): 1, 1 | ||
// REVERT # Argument encoding is not properly implemented, yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the comment stay after a call to "update"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier to add the comment to the function call and not the the expectation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, in general, it might make sense to allow newlines (which will be preserved during update) in the list of arguments. Is that supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically this would mean that function call is everything up to a line that starts with REVERT
or ->
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New lines for function calls are not supported as of now. To clarify, your proposal is to have the call and the expectations written down like this:
// f(uint256)
// 1
// -> 2
// g(uint256, uint256)
// 1, 1
// REVERT # Argument encoding is not properly implemented, yet.
Is that correct? I'd also slightly tend towards that solution as it would have the benefit to support not only Ether that is send (currently: f(uint256): 1 [etherValue]
), but also argument ranges that I'd like to implement at some point:
// f(uint256, uint256): [etherValue]
// [1...10], 1
// -> 2
In my opinion, this would be consistent as everything that is send with the function call (arguments and ether) is encoded in brackets. But maybe that's a bit confusing and we should keep the ether value in brackets only. Supporting argument ranges could simply work like this: 1..10, 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier to add the comment to the function call and not the the expectation.
The comment is preserved on both lines already. So I guess it's fine for now. Would need to make sure that this works if we're going to add another extra line for the arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was suggesting a relaxed handling of newline characters: I would assume that sometimes, the arguments can be rather large and the argument list long, so you would want to be able to insert newlines at convenient places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the comment again, I also got your intention.
I'm sorry, I think it was also my fault that this PR got so big. Are there reasonable parts to extract into other PRs? |
Yea, that's the reason why I left out some aspects in the first place. One the other hand it's a good opportunity to split out the expectation parser and properly test it. The other part will then be the actual execution environment. |
Closing this now in favor of #5860 and following. |
Description
Part of #4223.
Based on #4245.
This PR adds support for running semantic (end-to-end) tests via
isoltest
. It enables:uint
,int
(unpadded)A test that fails, creates an output of this form:
Not supported:
string
,bool
Todo
Follow-up PRs
no-ipc
is specified, at least compile tests