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

Semantic test infrastructure #5872

Merged
merged 2 commits into from
Feb 18, 2019
Merged

Semantic test infrastructure #5872

merged 2 commits into from
Feb 18, 2019

Conversation

erak
Copy link
Collaborator

@erak erak commented Jan 27, 2019

Part of #4223 and based on #5860.

Adds infrastructure to the testing environment (isoltest and boost unit tests, as well), that allows to run file-based semantic tests.

Checklist

@codecov
Copy link

codecov bot commented Jan 28, 2019

Codecov Report

Merging #5872 into develop will decrease coverage by 0.43%.
The diff coverage is 40.29%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5872      +/-   ##
===========================================
- Coverage    88.59%   88.16%   -0.44%     
===========================================
  Files          359      361       +2     
  Lines        34596    34774     +178     
  Branches      4089     4122      +33     
===========================================
+ Hits         30651    30657       +6     
- Misses        2581     2724     +143     
- Partials      1364     1393      +29
Flag Coverage Δ
#all 88.16% <40.29%> (-0.44%) ⬇️
#syntax 27.71% <4%> (-0.23%) ⬇️

@erak erak self-assigned this Jan 28, 2019
@erak erak force-pushed the semantic-tests-split branch from 077b788 to db34d02 Compare February 1, 2019 07:39
@erak erak force-pushed the test-file-parser branch 6 times, most recently from 0fdfefc to 83eb987 Compare February 2, 2019 15:14
@erak erak mentioned this pull request Feb 4, 2019
@erak erak force-pushed the test-file-parser branch 2 times, most recently from ad80816 to f5fa62d Compare February 5, 2019 12:57
@erak erak force-pushed the semantic-tests-split branch 6 times, most recently from ac13d28 to 4e34426 Compare February 6, 2019 13:13
@ethereum ethereum deleted a comment from stackenbotten Feb 6, 2019
@erak erak force-pushed the semantic-tests-split branch from 4e34426 to 3e173ac Compare February 6, 2019 15:51
@erak erak force-pushed the test-file-parser branch from bda5619 to 9c97937 Compare February 6, 2019 15:56
@erak erak force-pushed the semantic-tests-split branch from 3e173ac to 8db2dfa Compare February 6, 2019 15:57
@erak erak force-pushed the test-file-parser branch from 9c97937 to a10982e Compare February 6, 2019 15:59
@erak erak force-pushed the semantic-tests-split branch from 8db2dfa to bde513d Compare February 6, 2019 16:03
@erak erak force-pushed the test-file-parser branch from a10982e to c9c4578 Compare February 6, 2019 16:26
@erak erak force-pushed the semantic-tests-split branch 2 times, most recently from 5a4e9e5 to b5a8662 Compare February 6, 2019 16:55
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Most of it looks good, but still, I think we should restructure it, so that only the result encoding is regenerated and the call and argument part is not touched at all (plus some minor other stuff).

bool disableSMT = false;
bool formatted = true;
po::options_description options(
R"(isoltest, tool for interactively managing test contracts.
Usage: isoltest [Options] --testpath path
Usage: isoltest [Options] --testpath path --ipcpath ipcpath
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Usage: isoltest [Options] --testpath path --ipcpath ipcpath
Usage: isoltest [Options] --testpath path {--ipcpath ipcpath, --no-ipc}

Or [--ipcpath ipcpath]in "optional brackets"?

Or just two lines (with or without repeating Usage: ):

Usage: isoltest [Options] --testpath path --ipcpath ipcpath
Usage: isoltest [Options] --testpath path --no-ipc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could follow the unix standard here and print:

Usage: isoltest [--testpath testpath] [--ipcpath ipcpath] [--no-ipc]

or

Usage: isoltest [--no-ipc] [--testpath testpath] [--ipcpath ipcpath]

Copy link
Member

Choose a reason for hiding this comment

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

Actually if we start guessing the ipcpath, then none of the options are particularly special and we could just leave it at Usage: isoltest [Options]. In any case we can remove the --testpath testpath part here - I think it's only in there as a relic from the time where it wasn't optional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've gone with Usage: isoltest [Options] --ipcpath ipcpath for now, since we're not guessing it, yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that "guessing" code needs to be shared with soltest, @Marenz what's your take on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Saw this too late. But yeah, if they share command line parameters the defaults / handling should be shared

@erak erak changed the base branch from test-file-parser to develop February 11, 2019 10:08
@erak erak force-pushed the semantic-tests-split branch from b5a8662 to 6ac44b9 Compare February 11, 2019 22:31
@erak erak force-pushed the semantic-tests-split branch from 6ac44b9 to 90289ee Compare February 14, 2019 16:28
@erak
Copy link
Collaborator Author

erak commented Feb 15, 2019

Updated. This PR should now have all the features planned and can be reviewed again. Please note that all the test files have been updated by isoltest so this can be considered as the format spec that it follows. This formatting should now also be inline with the display-modes that the parser detects.

@erak
Copy link
Collaborator Author

erak commented Feb 15, 2019

Ah, I did not squash on purpose yet, but will do when it's ready to be merged.

ekpyron
ekpyron previously approved these changes Feb 15, 2019
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Yeah - a lot of this needs to be touched again anyways to add more features and we can improve some details then - I'd say this is fine, once it's squashed!

// g(uint256,uint256): 1, -2 -> 3
// h(), 1 ether -> 1
// j() -> FAILURE
// i() # Does not exist. # -> FAILURE # Reverts. #
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? Why not -> FAILURE?

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 what it says I think - The second # marks the end of an inner-line-comment I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see!

@chriseth chriseth merged commit 968ca88 into develop Feb 18, 2019
@ekpyron ekpyron deleted the semantic-tests-split branch February 18, 2019 17:20
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.

4 participants