Feedback and opinions

[paldepind]

Hello @danr. I think this library looks really cool and very useful. I'm very intersted in using it myself. Below is a braindump of feedback and various opinions. I hope some it might be useful to you.

First of all, I think this library is very promising. Property-based testing is super useful but so far no JS library has made that available in an easily accessible way. JSVerify has come closest but as far as I can tell it hasn't reached an audience wider than the "JavaScript programmers who love Haskell" niche. I think a big part of the reason is:

1. The API includes writing Haskell type declarations inside strings. Writing code inside strings isn't nice and the Haskell syntax is off-putting to most JS developers.
2. The documentation isn't good enough.

I think ts-quickcheck can bring a lot of value to JavaScript and gain a lot of traction if it nails the API and becomes reasonably documented.

In my opinion, the API should strive to be as simple and concise as possible.

I find this to be quite heavy handed and tricky to read.

assert.isTrue(
  QC.QuickCheckStdout(
    Gen.range(2).array(),
    (m, p) => equals(concat(list(), list(...xs)), list(xs))
  )
);

To me this is much more concise and easy to understand.

QC.forall(QC.array, (xs) => equals(concat(list(), list(...xs))));

forall is much shorter than QuickCheck and I avoid having to type Gen. If forall indicates errors by throwing an exception I could easily use forall inside any testing framework.

With regards to the implementation, I must say I personally think some of the code is quite unidiomatic JavaScript. It looks more like how a Java/C# programmer whould write TyepScript than how a JavaScript programmer whould write TypeScript.

Here are a few of the things that I noticed:

• Variable names with underscores. shrink_number, etc. In JavaScript, variables are most often in camelCase.
• Function names that begin with a capital. Like PrintTestResult, QuickCheck, etc. This is bad practice. Many JavaScript programmers use the convention that only class names begin with a capital letter. The convention is by default upheld by many linters. For instance this ESLint rule. This means that many programmers will get a lint error if the call QuickCheck because their linter assumes QuickCheck is a class and it will tell them to add a new in fron of QuickCheck.
• Overruse of classes and static methods. IMO static methods are rarely desireable in JavaScript. They're nececarry in Java. But in JavaScript we can just export a function instead. Functions are easier to use by consumers of a library. Instead of typing QC.Gen.foo one can simply type QC.foo which is shorter. Exported function can also more easily be imported directly into the scope with import {foo} from "ts-quickcheck". Additionally, functions can be completely private, unlike static methods. Even though _between is private on on Gen JavaScript consumers can still reach it. _between could easily be converted to a function and then it would be completely private.

Can you point me towards a paper or something similar that explains the implementation techniques behind the library? I'm still trying to grok how the code works.

Anyway, that was just a couple of opinions. I'll very much be looking forward to how this library progresses :smile:

[danr]

Hi @paldepind, thanks a lot for the write up!

Regarding these three points:

In my opinion, the API should strive to be as simple and concise as possible.

Overruse of classes and static methods.

QC.forall(QC.array, (xs) => equals(concat(list(), list(...xs)))); forall is much shorter than QuickCheck and I avoid having to type Gen.

I agree that the API should be simple and concise :)

I wanted users to be able to import the Gen type and its (static) methods in one go. I also don't know how to reexport functions from another file, so I have found it convenient to just attach them to the Gen class.

Another reason why I make the methods static is because I want the constructor of Gen to be private and then I can only access it using static in TypeScript.

With this background, do you see a way to export the Gen type along with utility functions from the main entry point of the library? Here's an incomplete sketch:

src/Gen.ts:
    export class Gen<A> { ... } 
        // or
    export interface Gen<A> { ... }

    export function between(lo: number, hi: number): Gen<A> { ... }
    ...

src/main.ts:
    // import & rexport contents of src/Gen.ts, including the Gen type

    // your forall function 
    export function forall<A>(g: Gen<A>, (a: A) => boolean) { ... }

With you importing the entire library as QC, how do you want to refer to the Gen type? Using QC.Gen?

If forall indicates errors by throwing an exception I could easily use forall inside any testing framework.

I will expose such a function.

Variable names with underscores. shrink_number, etc. In JavaScript, variables are most often in camelCase. Function names that begin with a capital. Like PrintTestResult, QuickCheck, etc. This is bad practice.

I just find camelCase ugly and I'm happy I'm not writing Haskell which (essentially) enforces it. I can try to use camelCase, at least for exported functions :P

Even though _between is private on on Gen JavaScript consumers can still reach it. _between could easily be converted to a function and then it would be completely private.

This confusion for JavaScript consumers is good point against static methods, thanks.

Can you point me towards a paper or something similar that explains the implementation techniques behind the library? I'm still trying to grok how the code works.

I don't think there is an article with this particular implementation technique. The idea is to make use a reader transformer with the random number generator and the current size as environment over a (lazy) rose tree, which represents the generated value and potential shrinks. When shrinking starts it does a depth-first search over the tree until the property is no longer false.

The Haskell QuickCheck internally has such a tree but it is not exposed when making generators. There are clean implementations in the code bases of Hedgehog and purescript-jack (which seems to be quite a faithful PureScript translation).

[paldepind]

I wanted users to be able to import the Gen type and its (static) methods in one go. I also don't know how to reexport functions from another file, so I have found it convenient to just attach them to the Gen class.

Here is how you can do it

// Gen.ts

export function char() {
  // ...
}
function _between(..) { // Not exported, completely private both statically and at run-time
  // ...
}
export function between(lo: number, hi: number): Gen<number> {
  // ...
}

// main.ts

// reexport everything from Gen.ts
export * from './Gen'
// alternatively one can decide what get reexported
export {char, between} from './gen'

The benefit is that then people can easily choose how they want to import things:

import * as QC from 'ts-quickcheck' // import into namespace
import {between, forall} from 'ts-quickcheck' // get most used stuff directly in scope

Another reason why I make the methods static is because I want the constructor of Gen to be private and then I can only access it using static in TypeScript.

Good point. I ran into the same problem in List. The List constructor should be private but I can't since I use the constructor in some internal functions. But there is only a single constructor and there can be many internal functions. So I'd rather make the single constructor public to gain the benefit that I can make all the internal function truly private.

With this background, do you see a way to export the Gen type along with utility functions from the main entry point of the library?

Please let me know if what I wrote above is what you had in mind.

With you importing the entire library as QC, how do you want to refer to the Gen type? Using QC.Gen?

Yes, exactly.

I will expose such a function.

I'll definitely be looking forward to that. I'm very interested in using the library to test List.

I just find camelCase ugly and I'm happy I'm not writing Haskell which (essentially) enforces it. I can try to use camelCase, at least for exported functions :P

That sounds like a good tradeof :+1:

I don't think there is an article with this particular implementation technique.

I'll be looking more closely at your implementation then.

[danr]

I now pushed a commit which makes it possible to import all generator combinators in the same namespace as forall and similar functions. It should now be possible to write something like so:

import * as QC from 'ts-quickcheck'
QC.forall(QC.nat.replicate(2), ([x,y]) => x+10 > y)

I find it a bit confusing not to use the Gen prefix for generators so I'm importing two copies of the namespace now, as QC and as Gen, in the tests right now.

[paldepind]

It looks really good to me :+1:

I'll close the issue.