Skip to content

Fixed bug in FingerTree.append(). Added PropertyTestRunner #144

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.

Sign up for GitHub

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

Jump to bottom
Merged
mperry merged 5 commits into from
Jun 4, 2015
Merged

Fixed bug in FingerTree.append(). Added PropertyTestRunner #144

mperry merged 5 commits into from
Jun 4, 2015

Conversation

@orionll
Copy link
Contributor

@orionll orionll commented Jun 1, 2015

Guys, I have a suggestion. FunctionalJava has a decent property based testing framework (fj.test), but it is not used in FunctionalJava. Instead, ScalaCheck is used. I think, we should rewrite all properties to fj.test and remove ScalaCheck dependency.

Here are some advantages of this approach:

  • Test classes will be a good example of fj.test usage for programmers that use FunctionalJava.
  • It will test fj.test.* classes themselves.
  • Programmers will trust fj.test more.
  • Scala dependency will be redundant.

Don't get me wrong. I like Scala, but FunctionalJava is a library for Java and it should use Java as much as possible.

I've implemented PropertyTestRunner which integrates JUnit with fj.test. All classes that are marked with RunWith(PropertyTestRunner) will be executed during the Gradle build. SeqProperties is an example of a class with properties (you can run it directly from IDEA too).

I'm open for discussion.

@jbgi
Copy link
Member

jbgi commented Jun 1, 2015

I think this a good idea overall. some considerations that may be worth taking into account:

  1. since the merge of Add optics laws in tests and configure tests module for publishing #131, test is now a published artefact. Should the property-based testing framework be moved into that module? (as it is usually not needed at runtime)
  2. Junit as now some support for property-based testing (see "Junit theories" and https://github.com/pholser/junit-quickcheck). I don't know how good it is (looks like it relies on annotations a bit too much) but maybe we should just use that??

@orionll
Copy link
Contributor Author

orionll commented Jun 1, 2015

  1. I'm not sure. functionaljava-tests is a module that integrates ScalaCheck with FunctionalJava, right?
  2. I've just looked at junit-quickcheck. Looks like an imperative enterprise library. There is no spirit of functional programming in it :) (fj.test is based on purity, monads etc.)

@jbgi
Copy link
Member

jbgi commented Jun 1, 2015

  1. My broader concern is: should the property-based testing stuff be in a separate artifact? (IMO: yes). If we get ride of scalacheck dep then functionaljava-tests could be that artifact (but this is an operational detail).
  2. You are probably right about junit-theories.

@orionll
Copy link
Contributor Author

orionll commented Jun 1, 2015

  1. Totally agree. Modularizing is always a good idea.

orionll added 2 commits June 2, 2015 14:29
@orionll
Fixed bugs in standard random generator, Gen.listOf(). Added Arbitrar…
69b3865 
…y.arbNonEmptyList(), NonEmptyList.tail(), NonEmptyList.head(), NonEmptyList.equals(), NonEmptyList.toString(), NonEmptyListProperties.
@orionll
Added NonEmptyList.hashCode(), Hash.bigintHash, Hash.bigdecimalHash
c2a126a
orionll
orionll reviewed Jun 2, 2015
View reviewed changes
core/src/main/java/fj/test/Rand.java
Copy link
Contributor Author

@orionll orionll Jun 2, 2015

Choose a reason for hiding this comment

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

Let me explain why I changed these lines. There were two bugs here:

  • The upper bound was exclusive, but it should be inclusive.
  • It didn't work for ranges wider than Integer.MAX_VALUE (e.g. range [-10; Integer.MAX_VALUE]).

orionll added 2 commits June 3, 2015 00:27
@orionll
Added methods for NonEmptyList: snoc(), reverse(), intersperse(), sor…
74103b4 
…t(), zipIndex(), zipWith(), unzip(), join(). Added variable arguments to NonEmptyList.nel().
@orionll
Added JavaDoc for new NonEmptyList methods.
9305748
mperry
mperry reviewed Jun 4, 2015
View reviewed changes
core/src/main/java/fj/test/Arbitrary.java
Copy link
Contributor

@mperry mperry Jun 4, 2015

Choose a reason for hiding this comment

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

Is this always a list of size 1, rather than some arbitrary length?

Copy link
Contributor Author

@orionll orionll Jun 4, 2015

Choose a reason for hiding this comment

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

No.

  • Gen.listOf - list of arbitrary length ([0, ∞)).
  • Gen.listOf1 - non empty list of arbitrary length ([1, ∞)).
  • Gen.sequenceN - list of specified length.

Copy link
Contributor

@mperry mperry Jun 4, 2015

Choose a reason for hiding this comment

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

You're right, but I had to dig through the code to convince myself this is true. The existing javadoc should be more specific.

mperry added a commit that referenced this pull request Jun 4, 2015
@mperry
Merge pull request #144 from orionll/master
369e9b6 
Fixed bug in FingerTree.append(). Added PropertyTestRunner
@mperry mperry merged commit 369e9b6 into functionaljava:master Jun 4, 2015
@mperry
Copy link
Contributor

mperry commented Jun 4, 2015

I think it is a good idea to redo the ScalaCheck property tests using FunctionalJava.

Originally the property testing framework was separate called Reductio and was merged into FunctionalJava. I think it makes sense to split it into a separate component. I have looked at the FJ property test classes previously and I think they matched the quickcheck paper by John Hughes (http://www.eecs.northwestern.edu/~robby/courses/395-495-2009-fall/quick.pdf). For this reason, I suggest we call the artifact functionaljava-quickcheck and put this into a quickcheck directory. The Scala and FJ property tests would live in separate components. For some context, I originally split the ScalaCheck tests into a separate module to assist IDEs. This made it easy to exclude the ScalaCheck tests where the IDE had poor Scala support.

I still have to do some playing with the PropertyTestRunner to understand all the details.

@orionll
Copy link
Contributor Author

orionll commented Jun 4, 2015

functionaljava-quickcheck - good name, I like it.

@mperry mperry mentioned this pull request Jun 4, 2015
Closed
@mperry
Copy link
Contributor

mperry commented Jun 4, 2015

I am going to try to split this out now. See #147.

@mperry mperry added this to the v4.4 milestone Jul 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug enhancement

Projects

None yet

Milestone

v4.4

Development

Successfully merging this pull request may close these issues.

3 participants

@orionll @jbgi @mperry