Integrate rapidcheck into pony code base test framework #1840

WojciechMigda · 2017-04-15T18:24:14Z

Rapidcheck revision 5571b312a90efec90b3686f9faa8b6a2a21b63a9 was
placed into lib/rapidcheck.

Pony's Makefile was modified to compile rapidcheck code and link
tests against it. This includes ability to compile *.cpp files.

Sample test suite was created to demonstrate that wiring is in place.
File fun-rc.cc was created in test/libponyrt/ds to implement some
of the test cases found in fun.cc.

Thing to note:
Rapidcheck requires enabled rtti. To avoid recompiling things twice
three minor changes were introduced into rapidcheck code:
a) include/rapidcheck/detail/Any.hpp
virtual method typeInfo() which returns typeid(T) was commented
out,
b) include/rapidcheck/detail/ShowType.hpp
printing typeid(T).name() to the io stream was commented out,

I would like to ask for feedback on few items:

disabled rtti. I have commented few lines in rapidcheck to accomodate this but it is far from being a long term solution.
tests organisation, whether tests which utilize rapidcheck should exist in the same locations as the other ones (means all tests are linked against rapidcheck) or there should be a separate rapidcheck-dedicated target, the only one which is linked against the library.
possible tests naming convention to indicate those which utilize rapidcheck.

Thank you,
Wojciech

Rapidcheck revision 5571b312a90efec90b3686f9faa8b6a2a21b63a9 was placed into lib/rapidcheck. Pony's Makefile was modified to compile rapidcheck code and link tests against it. This includes ability to compile *.cpp files. Sample test suite was created to demonstrate that wiring is in place. File fun-rc.cc was created in test/libponyrt/ds to implement some of the test cases found in fun.cc. Thing to note: Rapidcheck requires enabled rtti. To avoid recompiling things twice three minor changes were introduced into rapidcheck code: a) include/rapidcheck/detail/Any.hpp virtual method typeInfo() which returns typeid(T) was commented out, b) include/rapidcheck/detail/ShowType.hpp printing typeid(T).name() to the io stream was commented out,

SeanTAllen · 2017-04-17T23:06:07Z

@WojciechMigda there appear to be some compilation errors.

killerswan · 2017-04-19T19:46:37Z

What's RapidCheck's license? Is there a LICENSE file that should be in this?

SeanTAllen · 2017-04-19T19:51:51Z

@WojciechMigda THANK YOU!

jemc · 2017-04-19T19:56:12Z

@WojciechMigda - Some feedback from today's sync call:

We're generally amenable to this, but more so for the runtime (libponyrt) than for the compiler (libponyc). That is, we'd like to see C++ property-based testing for things like the libponyrt hashmap implementation, but not so much for things like subtyping and ASTs. This is partly because we think that such tests will be more easily written in Pony, when we finally have some of the compiler stuff written in Pony. This is just something to keep in mind for the future when writing more property-based tests, and doesn't affect the mergability of this current PR.

Responding to your specific 3 questions:

Let's turn on RTTI, but only for the rapidcheck-dedicated target (see point 2).
Let's use a rapidcheck-dedicated target.
If it's in a rapidcheck-dedicated target, we think a name convention probably isn't necessary.

Also, can you please address @killerswan's comment about the license?

Thanks!

WojciechMigda · 2017-04-19T20:37:10Z

Hi guys,
thank you for looking at this PR.
I was going to jump back in once I sort out the build errors but now I'll try to address your comments.

re: rtti. It can be enabled for the target which builds librapidcheck, but... there are many ways in which one can write testcases with rapidcheck. The most basic is when scenarios are written using rapidcheck api directly. Then there are provided wrappers for gtest, check, and boost.test. If one would like to use gtest wrapper (which would allow seamless integration with existing pony(rt) tests written with gtest) then the rtti thing be becomes an issue as rapidcheck expects that google test would be compiled with the same rtti setting. Hence, provided I am not missing something obvious, we would need libgtest being compiled twice - first for existing testcases which require rtti disabled, and then for rapidcheck which needs the opposite. I haven't checked the latter, though, so it might turn out that we would trip over ponyrt in turn not being compiled with rtti on.
One thing I can do to put this forward is to leave librapidcheck compiled with rtti, drop the sample scenarios in fun-rc.cc, and leave the decision about how rapidcheck testcases should be written (vanilla, gtest, ...) open. This should probably also help resolve compiler errors, although I am still baffled why they did not pop up at my setup.
License: my bad, I missed it. It is BSD-2 clause simplified (https://github.com/emil-e/rapidcheck/blob/master/LICENSE.md).

I'll try to do the following for now:

enable rtti for librapidcheck,
include rapidcheck license in the rapidcheck's subfolder,
remove fun-rc.cc and try to create a separate target wih plain vanillla example testcase.

SeanTAllen · 2017-04-19T20:54:33Z

@WojciechMigda if you create a rapidcheck-test Makefile rule, you should be able to enable rtti in general for the pony runtime and everything else.

jemc · 2017-04-19T21:10:06Z

remove fun-rc.cc and try to create a separate target wih plain vanillla example testcase.

why this step?

WojciechMigda · 2017-04-19T21:56:56Z

why this step?
@jemc Currently fun-rc.cc contains tests which make use of the gtest wrapper for rapidcheck. If I enable rtti for librapidcheck then without libgtest compiled with rtti too it won't link. With @SeanTAllen 's suggestion above, if I understand it correctly, we'll get it working, but with some stuff compiled twice, with and without rtti.

jemc · 2017-04-19T22:34:54Z

Got it. Thanks for the extra info.

For whatever it's worth, it would be okay to enable RTTI for the gtest targets - I think we just wanted to avoid compiling ponyc itself with RTTI if possible.

If you were being facetious, you could describe the Pony runtime as a series of hashmaps that are held together by some code. Hash performance and correctness can have a great impact on everything else in the runtime because they are at the basis of most everything else in the runtime. This change fixes a number of issues that appears to be garbage collection bugs but were in fact, problems with invariant violation in the underlying hash implementation. It should be noted that while the rest of this comment discuss invariant violations that exist in our Robin Hood hash implementation, some of the bugs that this closes predate the Robin Hood implementation. This leads me to believe that the previous implementation had some subtle problem that could occur under some rare interleaving of operations. How this occurred is unknown at this time and probably always will be unless someone wants to go back to the previous version and use what we learned here to diagnose the state of the code at that time. This patch closes issues #1781, #1872, and #1483. It's the result of teamwork amongst myself, Sylvan Clebch and Dipin Hora. History should show that we were all involved in this resolution. The skinny: When garbage collecting items from our hash, that is, removing deleted items to free up space, we can end up violating hash invariants. Previously, one of these invariants was correctly fixed, however, it incorrectly assumed another invariant held but that is not the case. Post garbage collection, if any items have been deleted from our hash, we do an "optimize" operation on each hash item. We check to see if the location the item would hash to is now an empty bucket. If it is, we move the item to that location thereby restoring the expected chaining. There is, however, a problem with doing this. It's possible over time to violate another invariant when fixing the first violation. For a given item at a given location in the hash, each item has a probe value. An invariant of our data structure is that items at earlier locations in the hash will always have an equal or lower probe value for that location than items that come later. For example, items: "foo" and "bar". Given a hashmap whose size is 8, where "foo" would made to index 1 and "bar" would map to index "2". When looking at the probe values for "foo" and "bar" at index 1, "foo" would have a probe value of "0" as it is at the location it hashes to whereas "bar" would have a probe value of "7". The value is the number of indexes away from our "natural" hash index that the item is. When search the hash, we can use this probe value to not do a linear search of all indexes for the a given key. Once we find an item whose probe value for a given index is higher than ours, we know that the key can't be in the map past that index. Except our course for when we are restoring invariants after a delete. It's possible, due to the sequential nature of our "optimize" repair step, to violate this "always lower probe value" invariant. The previous implementation of "optimize_item" assumed that in invariant held true. By not detecting the invariant violation and fixing it, we could end up with maps where a key existed in it but it wouldn't be found. When the map in question was an object map used to hold gc'able items, this would result in an error that appears to be a gc error. See #1781, #1872, and #1483. It should be noted, that because of the complex chain of events that needs to occur to trigger this problem that we were unable to devise a unit test to catch this problem. If we had property based testing for the Pony runtime, this most likely would have been caught. Hopefully, PR #1840 to add rapidcheck into Pony happens soon. Closes #1781 Closes #1872 Closes #1483

SeanTAllen · 2017-05-04T11:29:28Z

@WojciechMigda we are about to close a bug that could have been found via automated means if we had property based testing of the runtime. Looking forward to this making its way in.

WojciechMigda · 2017-05-04T11:30:37Z

@SeanTAllen I remember about this, I just need to find a slot to get back on the topic.

If you were being facetious, you could describe the Pony runtime as a series of hashmaps that are held together by some code. Hash performance and correctness can have a great impact on everything else in the runtime because they are at the basis of most everything else in the runtime. This change fixes a number of issues that appears to be garbage collection bugs but were in fact, problems with invariant violation in the underlying hash implementation. It should be noted that while the rest of this comment discuss invariant violations that exist in our Robin Hood hash implementation, some of the bugs that this closes predate the Robin Hood implementation. This leads me to believe that the previous implementation had some subtle problem that could occur under some rare interleaving of operations. How this occurred is unknown at this time and probably always will be unless someone wants to go back to the previous version and use what we learned here to diagnose the state of the code at that time. This patch closes issues #1781, #1872, and #1483. It's the result of teamwork amongst myself, Sylvan Clebch and Dipin Hora. History should show that we were all involved in this resolution. The skinny: When garbage collecting items from our hash, that is, removing deleted items to free up space, we can end up violating hash invariants. Previously, one of these invariants was correctly fixed, however, it incorrectly assumed another invariant held but that is not the case. Post garbage collection, if any items have been deleted from our hash, we do an "optimize" operation on each hash item. We check to see if the location the item would hash to is now an empty bucket. If it is, we move the item to that location thereby restoring the expected chaining. There is, however, a problem with doing this. It's possible over time to violate another invariant when fixing the first violation. For a given item at a given location in the hash, each item has a probe value. An invariant of our data structure is that items at earlier locations in the hash will always have an equal or lower probe value for that location than items that come later. For example, items: "foo" and "bar". Given a hashmap whose size is 8, where "foo" would made to index 1 and "bar" would map to index "2". When looking at the probe values for "foo" and "bar" at index 1, "foo" would have a probe value of "0" as it is at the location it hashes to whereas "bar" would have a probe value of "7". The value is the number of indexes away from our "natural" hash index that the item is. When search the hash, we can use this probe value to not do a linear search of all indexes for the a given key. Once we find an item whose probe value for a given index is higher than ours, we know that the key can't be in the map past that index. Except our course for when we are restoring invariants after a delete. It's possible, due to the sequential nature of our "optimize" repair step, to violate this "always lower probe value" invariant. The previous implementation of "optimize_item" assumed that in invariant held true. By not detecting the invariant violation and fixing it, we could end up with maps where a key existed in it but it wouldn't be found. When the map in question was an object map used to hold gc'able items, this would result in an error that appears to be a gc error. See #1781, #1872, and #1483. It should be noted, that because of the complex chain of events that needs to occur to trigger this problem that we were unable to devise a unit test to catch this problem. If we had property based testing for the Pony runtime, this most likely would have been caught. Hopefully, PR #1840 to add rapidcheck into Pony happens soon. Closes #1781 Closes #1872 Closes #1483

In the previous commit rtti was disabled for librapidcheck. With this change -fno-rtti flag is removed from libgtest, librapidcheck, and libponyrt.tests targets. In addition, license file is included with librapidcheck.

WojciechMigda · 2017-05-06T22:59:02Z

Appveyor build fails because seed value printed to stderr by rapidcheck testcases confuses waf(?). I would use some help on that, thanks.

chalcolith · 2017-05-07T00:07:01Z

I'll take a look in the next couple of days.

chalcolith · 2017-05-08T21:58:19Z

There can be problems using Python with PowerShell; try using cmd instead:

diff --git a/.appveyor.yml b/.appveyor.yml
index 5b494c1..9343732 100644
--- a/.appveyor.yml
+++ b/.appveyor.yml
@@ -84,6 +84,5 @@ build:
  none

test_script:
-  - ps: |
-      cd C:\projects\ponyc
-      python -x waf test --config $env:configuration --llvm $env:llvm
+  - cd C:\projects\ponyc
+  - python -x waf test --config %configuration% --llvm %llvm%

WojciechMigda · 2017-05-09T23:34:06Z

@kulibali thank you, it helped!

SeanTAllen · 2018-06-20T19:43:56Z

@WojciechMigda can you rebase this against master so we can get it moving towards being merged?

WojciechMigda · 2018-06-21T20:16:23Z

@SeanTAllen I am looking into this.

WojciechMigda · 2018-06-21T20:27:58Z

There was an issue submitted to rapidcheck (now closed) which reports this compilation error. From the quick glance it appears to be caused by a bug in gcc itself.
emil-e/rapidcheck#148
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41933

WojciechMigda · 2018-06-22T09:16:02Z

I tried three gcc compilers: 5.4.0 and 4.9.3 succeed, 4.8.5 fails. I will check if this can be worked around.

jemc · 2018-11-06T17:46:36Z

@WojciechMigda - any update on this one?

WojciechMigda · 2018-11-13T09:57:46Z

Unfortunately, no. However, I looked into the latest build logs above and it is confusing why llvm compiles the offending file on alpine images, but fails on others. Are these official llvm releases being used?

SeanTAllen · 2020-05-12T22:53:01Z

Closing as stale.

jemc added the needs discussion during sync label Apr 16, 2017

SeanTAllen mentioned this pull request Apr 16, 2017

Last Week in Pony - April 17, 2017 ponylang/ponylang-website#55

Closed

SeanTAllen removed the needs discussion during sync label Apr 19, 2017

SeanTAllen mentioned this pull request Apr 29, 2017

Possible #1872 issue fix #1876

Closed

Merge branch 'master' into cl/rapidcheck

9f46795

WojciechMigda added 3 commits May 6, 2017 19:59

Merge branch 'master' into cl/rapidcheck

7a55093

Enable rtti for some test targets

125a115

In the previous commit rtti was disabled for librapidcheck. With this change -fno-rtti flag is removed from libgtest, librapidcheck, and libponyrt.tests targets. In addition, license file is included with librapidcheck.

Align appveyor wscript for rapidcheck

05967db

WojciechMigda force-pushed the cl/rapidcheck branch from ece5bdd to 05967db Compare May 6, 2017 22:12

Switch appveyor from PowerShell to cmd

17ebba6

WojciechMigda force-pushed the cl/rapidcheck branch from 6f090c7 to 17ebba6 Compare May 9, 2017 23:02

SeanTAllen added needs discussion during sync labels Jun 12, 2018

SeanTAllen removed the needs discussion during sync label Jun 20, 2018

Merge branch 'master' into cl/rapidcheck

9b4026f

SeanTAllen removed the stale label May 12, 2020

SeanTAllen closed this May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate rapidcheck into pony code base test framework #1840

Integrate rapidcheck into pony code base test framework #1840

WojciechMigda commented Apr 15, 2017

SeanTAllen commented Apr 17, 2017

killerswan commented Apr 19, 2017

SeanTAllen commented Apr 19, 2017

jemc commented Apr 19, 2017 •

edited

Loading

WojciechMigda commented Apr 19, 2017

SeanTAllen commented Apr 19, 2017

jemc commented Apr 19, 2017

WojciechMigda commented Apr 19, 2017

jemc commented Apr 19, 2017

SeanTAllen commented May 4, 2017

WojciechMigda commented May 4, 2017

WojciechMigda commented May 6, 2017

chalcolith commented May 7, 2017

chalcolith commented May 8, 2017 •

edited

Loading

WojciechMigda commented May 9, 2017

SeanTAllen commented Jun 20, 2018

WojciechMigda commented Jun 21, 2018

WojciechMigda commented Jun 21, 2018

WojciechMigda commented Jun 22, 2018

jemc commented Nov 6, 2018

WojciechMigda commented Nov 13, 2018

SeanTAllen commented May 12, 2020

Integrate rapidcheck into pony code base test framework #1840

Integrate rapidcheck into pony code base test framework #1840

Conversation

WojciechMigda commented Apr 15, 2017

SeanTAllen commented Apr 17, 2017

killerswan commented Apr 19, 2017

SeanTAllen commented Apr 19, 2017

jemc commented Apr 19, 2017 • edited Loading

WojciechMigda commented Apr 19, 2017

SeanTAllen commented Apr 19, 2017

jemc commented Apr 19, 2017

WojciechMigda commented Apr 19, 2017

jemc commented Apr 19, 2017

SeanTAllen commented May 4, 2017

WojciechMigda commented May 4, 2017

WojciechMigda commented May 6, 2017

chalcolith commented May 7, 2017

chalcolith commented May 8, 2017 • edited Loading

WojciechMigda commented May 9, 2017

SeanTAllen commented Jun 20, 2018

WojciechMigda commented Jun 21, 2018

WojciechMigda commented Jun 21, 2018

WojciechMigda commented Jun 22, 2018

jemc commented Nov 6, 2018

WojciechMigda commented Nov 13, 2018

SeanTAllen commented May 12, 2020

jemc commented Apr 19, 2017 •

edited

Loading

chalcolith commented May 8, 2017 •

edited

Loading