Fix for counterexample crashing bug #327

[TysonMN]

Fixes #327

I think this PR fixes the bug where an exception isn't caught when checking a Property<_> containing counterexample. I got the idea for this fix by looking at Property.forall, which is the only other place that Property.counterexample is called. In the last commit, I removed the exception handling code from Property.forall since it is now happening in Property.bind.

How does this compare to the code in Haskell Hedgehog?

@AlexeyRaga, does the branch in this PR give the behavior that you expect?

[TysonMN]

The build fails because

Expecto_Expect_stringContains is not defined

when compiled by Fable. This is related to a test that I added.

@ThisFunctionalTom, do you have an opinion about how to fix this? Of course the brute force solution is to have Fable ignore this test.

[ThisFunctionalTom]

The build fails because

Expecto_Expect_stringContains is not defined

when compiled by Fable. This is related to a test that I added.

@ThisFunctionalTom, do you have an opinion about how to fix this? Of course the brute force solution is to have Fable ignore this test.

I guess we can make a pull request in mocha to also implement Expect.stringContains or change the implementation to Expect.isTrue (report.Contains.... I think the first option is preferable.

[ThisFunctionalTom]

I just made a pull request for it.
If you need more functions from Expect module we would probably need to copy the implementation from Expecto into Fable.Mocha but most of the time this should not be a problem I guess.

[TysonMN]

That PR seems like the right fix. Thanks @ThisFunctionalTom.

[ThisFunctionalTom]

If you are in a hurry I guess we could extend the Expect module in FSharp.Hedgehog with a #if FABLE guard until the PR is merged.

[TysonMN]

I don't think there is any hurry. I would prefer to wait for that PR to be merged and a release to be made.

[ThisFunctionalTom]

Sorry, have not got time for this. The pull request is already merged but we still have some troubles because we do not use the Except module from Mocha. I will look into it on the weekend.

[ThisFunctionalTom]

Finally had some time to look into it.

The first problem is that Fable.Mocha does not have all the assertions from the Expect module as Expecto does. The stringContains is now added but there are a lot more missing.

If we are using only a few assertions we can copy the implementation from Expecto into TestDsl (not a really big fan of this solution) or we can make a pull request in Expecto repository to add the Expect.fs file into .fable directory of the Expecto nuget package (the same way we did it in fsharp-hedgehog) so all functions from Expect will work out of the box with fable compiler. I looked into it and it seams to complicated to do. There would be a lot of #if !FABLE_COMPILER guards.

The second problem is that we must open the correct namespace depending on FABLE_COMPILER. In one case it is Expecto namespace with submodule Expect and in the other case it is Fable.Mocha namespace with submodule Expect. I tried to abstract this away with TestDsl module but it does not work because each test file must open its own namespaces.
We have 3 2 options here:

• use #if FABLE_COMPILER in each file and open different Expect modules like this

  # if FABLE_COMPILER
  open Fable.Mocha
  #else
  open Expecto
  #endif

• alias or implement all used assertions in TestDsl module like I did for isTrue and stringContains
• if we succeed in making Expecto more fable friendly by adding Expect.fs into .fable directory I guess it should work for most/all of the assertions from the Expect module.

The pull request with the fixed PropertyTests can be found here:
https://github.com/ThisFunctionalTom/fsharp-hedgehog/tree/bug/327_counterexample_crashes

How should we proceed? What do you think?

[TysonMN]

The first problem is that Fable.Mocha does not have all the assertions from the Expect module as Expecto does.

I would prefer if any assertions we need were added to Fable.Mocha.

The second problem is that we must open the correct namespace depending on FABLE_COMPILER. In one case it is Expecto namespace with submodule Expect and in the other case it is Fable.Mocha namespace with submodule Expect.

We don't have very many test files. I think it is fine to add the code in your comment to the top of each file.

[ThisFunctionalTom]

The first problem is that Fable.Mocha does not have all the assertions from the Expect module as Expecto does.

I would prefer if any assertions we need were added to Fable.Mocha.

The second problem is that we must open the correct namespace depending on FABLE_COMPILER. In one case it is Expecto namespace with submodule Expect and in the other case it is Fable.Mocha namespace with submodule Expect.

We don't have very many test files. I think it is fine to add the code in your comment to the top of each file.

OK. I will change my pull request for your pull request (:trollface:) accordingly.

Yeah, about that... I am not sure how much of Expecto Expect module should I copy to Fable.Mocha. Currently we are using just 2 assertions but I guess it will be more and if we always copy just 1-2 functions Zaid will not be very happy with us 😉

Would it be OK to copy the implementation of the assertions to our TestDsl.fs file into Expect module and once we have more functions in the module we can make a merge request for Fable.Mocha?

[TysonMN]

Would it be OK to copy the implementation of the assertions to our TestDsl.fs file into Expect module and once we have more functions in the module we can make a merge request for Fable.Mocha?

Yes. This seems fine.

[TysonMN]

The most recent commit in this branch adds a workaround for the Fable compiler so that the tests pass. This is now ready to be merged.

[ghost]

Found a few things, mostly around exposing new public API.

[TysonMN]

@adam-becker, I think I addressed all the comment in your review. I left them unresolved so that is easy for you to double check them.

When you are satisfied, I would prefer to squash the commits a bit. Then we can merge.

[TysonMN]

I decided to squash the commits. I think this is ready to merge.

[TysonMN]

Rebased on master (for clarity) even though there were no conflicts

[ghost]

Looks good to me! Thanks @TysonMN