Skip to content

Move more parser tests to syntax tests#4157

Merged
chriseth merged 1 commit intodevelopfrom
parser-tests
May 23, 2018
Merged

Move more parser tests to syntax tests#4157
chriseth merged 1 commit intodevelopfrom
parser-tests

Conversation

@axic
Copy link
Copy Markdown
Contributor

@axic axic commented May 17, 2018

No description provided.

@axic axic requested a review from chriseth May 17, 2018 07:05
@axic
Copy link
Copy Markdown
Contributor Author

axic commented May 17, 2018

Left a couple of tests in SolidityParser.cpp, which had different errors (TypeError or DeclarationError), but did fix a couple of them during the transition.

@@ -0,0 +1,5 @@
contract Foo {
function f(uint[] storage constant x, uint[] memory y) internal { }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

storage constant is invalid (Storage location has to be "memory" (or unspecified) for constants.). Should we leave it as an error or use constant rather on the memory argument?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would leave it like it is.

function fun(uint a) returns(uint r) { return a; }
}
)";
// with support of overloaded functions, during parsing,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should not be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can add them back, though they are not relevant since syntax tests run the analysis phase.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok right, I actually didn't read the comment, just saw that it was not retained :)

chriseth
chriseth previously approved these changes May 18, 2018
Copy link
Copy Markdown
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

Fine apart from the missing comments. I did not check the tests in details nor did I count them.

@axic
Copy link
Copy Markdown
Contributor Author

axic commented May 22, 2018

Added back the comment, but I think it is not needed there. I did manually verify the test cases (the proper ones removed and proper expectations, compared to the old version, but any extra eyes are welcome.

cc @bit-shift @leonardoalt

@erak
Copy link
Copy Markdown
Collaborator

erak commented May 22, 2018

@axic Looks good to me. I counted 52 tests that have been removed and 54 that have been added, so I guess there're all covered ;)
The only thing that caught my attention is that a lot of the tests have redundant expectations, e.g. No visibility specified. Defaulting to "public". I'm wondering if we should adjust the contracts under test such that the amount of redundant warnings is reduced and then explicitly checked in dedicated tests. But I think this can also be part of a separate discussion.

@chriseth
Copy link
Copy Markdown
Contributor

I would say we should merge this first and then update the tests to 0.5.0 syntax, i.e. add public and stuff. Of course only if that is not the issue under test.

@axic
Copy link
Copy Markdown
Contributor Author

axic commented May 23, 2018

I would say we should merge this first and then update the tests to 0.5.0 syntax, i.e. add public and stuff.

Yes, that would be the best approach, these are only moving test cases (mostly) verbatim, 0.5.0 fixes should be applied after.

@axic axic deleted the parser-tests branch May 23, 2018 13:17
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.

3 participants