New semantic tests extracted #6888

anurag-git · 2019-06-04T08:03:27Z

Description

Extracted and added semantic tests for

Transaction Status
Empty Contract
Smoke test for Range

Checklist

Code compiles correctly
All tests are passing
New tests have been created which fail without the change (if possible)
README / documentation was extended, if necessary
Changelog entry (if change is visible to the user)
Used meaningful commit messages

anurag-git · 2019-06-04T08:11:45Z

Partial fixes #4223

leonardoalt · 2019-06-04T08:44:27Z

There are some trailing whitespaces (please check the failing test)

leonardoalt · 2019-06-04T08:45:22Z

Where were these tests extracted from? Can they be removed from the cpp test file(s) accordingly?

anurag-git · 2019-06-04T09:22:23Z

There are some trailing whitespaces (please check the failing test)

Done

anurag-git · 2019-06-04T09:24:12Z

Where were these tests extracted from? Can they be removed from the cpp test file(s) accordingly?

@leonardoalt , This has been taken from SolidityEndToEndTest.cpp. I am not sure, if I can just delete them.
Also, any test to check the sanity of above file after removing the tests.

ekpyron · 2019-06-04T09:50:39Z

@anurag-git The main purpose of extracting the tests is to be able to remove them from SolidityEndToEndTest.cpp, so yes: please go ahead and do that - it'll also make it easier to review the extraction in the PR.

Regarding the sanity of SolidityEndToEndTest.cpp: If you remove an entire BOOST_AUTO_TEST_CASE(...) { ... } block, then you're fine. If it's only parts of the tests you can check, whether it still compiles and run build/test/tools/soltest with an instance of aleth running in the background to execute the end-to-end-tests, but I don't expect problems there and we'll see any issues in the CI tests running on the PR anyways.

codecov · 2019-06-04T10:12:30Z

Codecov Report

Merging #6888 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #6888      +/-   ##
===========================================
+ Coverage    86.99%   87.01%   +0.01%     
===========================================
  Files          427      427              
  Lines        40842    40810      -32     
  Branches      4829     4825       -4     
===========================================
- Hits         35532    35512      -20     
+ Misses        3715     3706       -9     
+ Partials      1595     1592       -3

Flag	Coverage Δ
#all	`87.01% <ø> (+0.01%)`	⬆️
#syntax	`25.31% <ø> (+0.01%)`	⬆️

anurag-git · 2019-06-04T10:13:43Z

@anurag-git The main purpose of extracting the tests is to be able to remove them from SolidityEndToEndTest.cpp, so yes: please go ahead and do that - it'll also make it easier to review the extraction in the PR.

Regarding the sanity of SolidityEndToEndTest.cpp: If you remove an entire BOOST_AUTO_TEST_CASE(...) { ... } block, then you're fine. If it's only parts of the tests you can check, whether it still compiles and run build/test/tools/soltest with an instance of aleth running in the background to execute the end-to-end-tests, but I don't expect problems there and we'll see any issues in the CI tests running on the PR anyways.

Thanks, changes done and pushed.

test/libsolidity/SolidityEndToEndTest.cpp

leonardoalt · 2019-06-04T11:22:58Z

Discussion from Gitter: this PR depends on the yet non-existing feature alsoViaYul that should run both with/without for the extracted tests

ekpyron · 2019-06-05T14:27:30Z

@anurag-git It might take a bit of time until we have a good solution for the ALSO_VIA_YUL part.

If you want to go ahead with this soon, there are two options: either you just don't extract tests that involve ALSO_VIA_YUL or you create two files for each of these tests, one regular version and one version suffixed "_via_yul", which sets

// ====
// compileViaYul: true
// ----
// <test expectations>

in the expectations (you can find examples of such tests in test/libsolidity/semanticsTests/viaYul/*).

anurag-git · 2019-06-06T06:38:02Z

@anurag-git It might take a bit of time until we have a good solution for the ALSO_VIA_YUL part.

If you want to go ahead with this soon, there are two options: either you just don't extract tests that involve ALSO_VIA_YUL or you create two files for each of these tests, one regular version and one version suffixed "_via_yul", which sets
// ====
// compileViaYul: true
// ----
// <test expectations>
in the expectations (you can find examples of such tests in test/libsolidity/semanticsTests/viaYul/*).

Took 2nd approach and pushed.

leonardoalt · 2019-06-06T08:26:11Z

I'm still not sure about this solution with 2 files.
Are we going to merge them back into a single file when we have the alsoViaYul option?

anurag-git · 2019-06-06T15:14:53Z

I'm still not sure about this solution with 2 files.
Are we going to merge them back into a single file when we have the alsoViaYul option?

@ekpyron , Would need your help here? I am ok with both options.

ekpyron · 2019-06-06T16:18:31Z

I think I'll create a way to run both kinds of tests at once after all - an imperfect, but working version will be ready shortly - I'll let you know when it's done, then you can continue based on that (sorry for the resulting delay).

ekpyron · 2019-06-06T19:07:50Z

@anurag-git We just merged #6916, so if you rebase this PR on top of the most current develop, you'll be able to use compileViaYul: also for all the tests that have ALSO_VIA_YUL in SolidityEndToEndTests.cpp and we don't need two files, but only one. Sorry for the back and forth in this matter :-)!

anurag-git · 2019-06-08T10:39:20Z

@anurag-git We just merged #6916, so if you rebase this PR on top of the most current develop, you'll be able to use compileViaYul: also for all the tests that have ALSO_VIA_YUL in SolidityEndToEndTests.cpp and we don't need two files, but only one. Sorry for the back and forth in this matter :-)!

I hope the changes are correct. Finally.

ekpyron · 2019-06-08T13:53:24Z

@anurag-git Thank you very much! One final thing before we can merge this: can you squash the PR into one single commit?

ekpyron

I cleanup up the damage this took during rebasing. Now it's fine to be merged, once the tests pass again!

ekpyron · 2019-06-09T12:44:30Z

test/libsolidity/SolidityEndToEndTest.cpp

-		}
-	)";
-	compileAndRun(sourceCode);
-	testContractAgainstCppOnRange("f(uint256)", [](u256 const& a) -> u256 { return a * 7; }, 0, 100);


By the way: for this particular test here, it's not a problem and I don't really mind, but this command actually tests every value between 0 and 100, whereas the extracted test only checks two values, namely 0 and 99.

So actually tests involving testContractAgainstCppOnRange should probably stay in SolidityEndToEndTest.cpp.

But yeah, in this particular case, it really doesn't matter, if the full range is checked or not, so it's fine.

Ok. I will keep in my mind for future changes

1. Transaction Status 2. Empty Contract 3. Smoke test for Range

anurag-git changed the title ~~New semantic tests for extracted~~ New semantic tests extracted Jun 4, 2019

ekpyron reviewed Jun 4, 2019

View reviewed changes

test/libsolidity/SolidityEndToEndTest.cpp Show resolved Hide resolved

anurag-git force-pushed the anurag_semantic_test_update branch from a04ef7b to 893d6b9 Compare June 6, 2019 06:36

anurag-git force-pushed the anurag_semantic_test_update branch from 893d6b9 to e38bf7d Compare June 6, 2019 07:07

ekpyron mentioned this pull request Jun 6, 2019

Allow extracted semantics tests to run with and without yul. #6916

Merged

ekpyron previously approved these changes Jun 8, 2019

View reviewed changes

anurag-git dismissed ekpyron’s stale review via 4fba476 June 9, 2019 04:54

anurag-git force-pushed the anurag_semantic_test_update branch from 3232090 to 4fba476 Compare June 9, 2019 04:54

ekpyron force-pushed the anurag_semantic_test_update branch from 4fba476 to 10b81f6 Compare June 9, 2019 12:35

ekpyron previously approved these changes Jun 9, 2019

View reviewed changes

Extract semantic tests for

60332c6

1. Transaction Status 2. Empty Contract 3. Smoke test for Range

ekpyron dismissed their stale review via 60332c6 June 9, 2019 12:46

ekpyron force-pushed the anurag_semantic_test_update branch from 10b81f6 to 60332c6 Compare June 9, 2019 12:46

ekpyron approved these changes Jun 9, 2019

View reviewed changes

ekpyron merged commit bd1f65d into ethereum:develop Jun 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New semantic tests extracted #6888

New semantic tests extracted #6888

anurag-git commented Jun 4, 2019

anurag-git commented Jun 4, 2019

leonardoalt commented Jun 4, 2019

leonardoalt commented Jun 4, 2019

anurag-git commented Jun 4, 2019

anurag-git commented Jun 4, 2019

ekpyron commented Jun 4, 2019

codecov bot commented Jun 4, 2019 •

edited

Loading

anurag-git commented Jun 4, 2019

leonardoalt commented Jun 4, 2019 •

edited

Loading

ekpyron commented Jun 5, 2019

anurag-git commented Jun 6, 2019

leonardoalt commented Jun 6, 2019

anurag-git commented Jun 6, 2019

ekpyron commented Jun 6, 2019

ekpyron commented Jun 6, 2019

anurag-git commented Jun 8, 2019

ekpyron commented Jun 8, 2019

ekpyron left a comment

ekpyron Jun 9, 2019

anurag-git Jun 10, 2019

New semantic tests extracted #6888

New semantic tests extracted #6888

Conversation

anurag-git commented Jun 4, 2019

Description

Checklist

anurag-git commented Jun 4, 2019

leonardoalt commented Jun 4, 2019

leonardoalt commented Jun 4, 2019

anurag-git commented Jun 4, 2019

anurag-git commented Jun 4, 2019

ekpyron commented Jun 4, 2019

codecov bot commented Jun 4, 2019 • edited Loading

Codecov Report

anurag-git commented Jun 4, 2019

leonardoalt commented Jun 4, 2019 • edited Loading

ekpyron commented Jun 5, 2019

anurag-git commented Jun 6, 2019

leonardoalt commented Jun 6, 2019

anurag-git commented Jun 6, 2019

ekpyron commented Jun 6, 2019

ekpyron commented Jun 6, 2019

anurag-git commented Jun 8, 2019

ekpyron commented Jun 8, 2019

ekpyron left a comment

Choose a reason for hiding this comment

ekpyron Jun 9, 2019

Choose a reason for hiding this comment

anurag-git Jun 10, 2019

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2019 •

edited

Loading

leonardoalt commented Jun 4, 2019 •

edited

Loading