Skip to content
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.

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

Property Method Attribute Generators #16

Merged
merged 35 commits into from
Jul 5, 2023

Conversation

JohnEffo
Copy link
Contributor

What is this

The aim of this pull request is to allow the definition of Generators via attributes. This has a couple of advantages, currently all generators for Properties need to be registered on the AutoGenConfig as discussed in your documentation:

type AutoGenConfigContainer =
  static member __ =
    GenX.defaults |> AutoGenConfig.addGenerator (Gen.constant 13)

This means that:

  • If a property wants to use different generators for the same type a wrapper type would need to be introduced e.g. PositiveInt
  • If different properties use different combinations of generators different AutoGenConfigContainer classes would need to be created.

Allowing generators to be specified at the parameter level allows a greater level of freedom at point of consumption, as the attributes themselves can take parameters, for example:

type IntCRange(max:int, min:int)=
  inherit ParameterGeneraterBaseType<int>()
  override this.Generator =  (Range.constant max min) |> Gen.int32

[<Property>]
let ``can restrict on range`` ([<IntCRange(min = 0, max =5 )>] i) =
  i >= 0 && i <= 5

I think this keeps the generator specification and tests closer together.

Work outstanding

This is a proof of concept, to see if you like the idea as such there is still work which need doing:

  • What parameters should come pre-baked
  • Documentation
  • If the type of the parameter and the attribute do not match an exception is thrown, tests do not show this at the moment.

@TysonMN
Copy link
Member

TysonMN commented Jun 18, 2023

Can you share a complete example here of a test as it would be written without this PR and then how it would be written with this PR?

@JohnEffo
Copy link
Contributor Author

In reply to @TysonMN. As I'm primarily a C# developer and I envisaged the usage of these attributes in C#, I have added the examples in C#, the code would be more succinct in F# but it think the same points would still apply.

I think that in both the attribute examples the code is more succinct. I also think that in an IDE it easier to find the generator by navigating to it's definition, in the first example you need to navigate to the Generators type and then definition then parse the type looking for the generator responsible for creating the type, in addition the same wrapper type could be defined in different Generator types with different definitions (In F# these would probably be generator function not associated with a type so this would not be an issue).

@TysonMN
Copy link
Member

TysonMN commented Jun 18, 2023

C# examples are fine.

Can you share the two versions of the test in a comment on this PR?

@JohnEffo
Copy link
Contributor Author

@TysonMN Here you.

Current Methodology

public record PositiveInt(int Value);
public record NegativeInt( int Value );

public class Generators
{
  public static Gen<PositiveInt> GenPositiveInt =>
    from x in Gen.Int32(Range.Constant(1, Int32.MaxValue))
    select new PositiveInt(x);

  public static Gen<NegativeInt> GenNegativeInt =>
    from x in Gen.Int32(Range.Constant(Int32.MinValue, -1))
    select new NegativeInt(x);

  public static AutoGenConfig _ => GenX.defaults
    .WithGenerator(GenPositiveInt)
    .WithGenerator(GenNegativeInt);
}

public class PositiveAndNegativeGeneratorContainerTypes
{
  [Property(typeof(Generators))]
  public bool ResultOfAddingPositiveAndNegativeLessThanPositive(
    PositiveInt positive,
    NegativeInt negative)
    => positive.Value + negative.Value < positive.Value;

}

Simple Attribute, non parameterized attribute

public class Negative : ParameterGeneraterBaseType<int>
{
  public override Gen<int> Generator => Gen.Int32(Range.Constant(Int32.MinValue, -1));
}
public class Positive : ParameterGeneraterBaseType<int>
{
  public override Gen<int> Generator => Gen.Int32(Range.Constant(1, Int32.MaxValue));
}

public class PositiveAndNegativeUtilizingIntegerRangeAttribute
{
  [Property]
  public bool ResultOfAddingPositiveAndNegativeLessThanPositive(
    [Positive] int positive,
    [Negative] int negative)
    => positive + negative < positive;
}

With Range Attribute

public class Int32Range : ParameterGeneraterBaseType<int>
{
  private readonly int _min;
  private readonly int _max;

  public Int32Range(int min, int max)
  {
    _min = min;
    _max = max;
  }
  public override Gen<int> Generator => Gen.Int32(Range.Constant(_min, _max));
}

public class PositiveAndNegativeWithAttributes
{
  [Property]
  public bool ResultOfAddingPositiveAndNegativeLessThanPositive(
    [Int32Range(1, Int32.MaxValue)] int positive,
    [Int32Range(Int32.MinValue, -1 )] int negative)
    => positive + negative < positive;
}

@dharmaturtle
Copy link
Member

My first impression is that I really like this since [Positive] int positive, [Negative] int negative is more composable than [Property(typeof(Generators))].

@JohnEfford
Copy link

My first impression is that I really like this since [Positive] int positive, [Negative] int negative is more composable than [Property(typeof(Generators))].

@dharmaturtle I thought so. Looking at my outstanding points

  • What parameters should come pre-baked
    Do you think we should add any default Attributes eg: Int32Range etc. Or leave it to the user to define all there own. If you think we should pre-bake some, could you suggest ones you feel would be essential and I'll get started on them?
  • Documentation
    Will start on this once I have more feed back on above.
  • If the type of the parameter and the attribute do not match an exception is thrown, tests do not show this at the moment.
    The exception that is thrown is pretty explicit, I'll highlight this possibility in the documentation when I write it.

@JohnEffo
Copy link
Contributor Author

Incase there is any confusion JohnEfford and JohnEffo are the same person, JohnEfford is my work GitHub account.

@TysonMN
Copy link
Member

TysonMN commented Jun 18, 2023

The examples look great. Thanks for sharing and creating this PR.

@JohnEffo
Copy link
Contributor Author

  • I made an executive decision not to have any baked-in attributes in this pull request.
  • I made a start on the documentation, Markdown for GitHub does not allow tabs to show separate code examples for F# and C#; would the below example, which shows the existing getting started section, be an acceptable format for everybody?

Getting Started

Install the Hedgehog.Xunit [package][nuget] from Visual Studio's Package Manager Console:

PM> Install-Package Hedgehog.Xunit

Please expand the appropriate code sample :)

F# ExampleSuppose you have a test that uses Hedgehog.Experimental and looks similar to the following:

open Xunit
open Hedgehog
[<Fact>]
let ``Reversing a list twice yields the original list`` () =
  property {
    let! xs = GenX.auto<int list>
    return List.rev (List.rev xs) = xs
  } |> Property.check

Then using Hedgehog.Xunit, you can simplify the above test to

open Hedgehog.Xunit
[<Property>]
let ``Reversing a list twice yields the original list, with Hedgehog.Xunit`` (xs: int list) =
  List.rev (List.rev xs) = xs
C# Example

Suppose you have a test that uses Hedgehog.Experimental and looks similar to the following:

using Hedgehog;
using Hedgehog.Linq;
using Hedgehog.Xunit;
using Property = Hedgehog.Linq.Property;

public class DocumentationSamples
{
  [Fact]
  public void Reversing_a_list_twice_yields_the_original_list()
  {
    var gen = GenX.auto<List<int>>();
    var prop = from data in Property.ForAll(gen)
      let testList = Enumerable.Reverse(data).Reverse().ToList()
      select Assert.Equivalent(data, testList, true);
    prop.Check();
  }
}

Then using Hedgehog.Xunit, you can simplify the above test to

[Property]
public void Reversing_a_list_twice_yields_the_original_list_with_xunit(List<int> xs)
{
    var testList = Enumerable.Reverse(xs).ToList();
    Assert.Equivalent(xs, testList, true);
}

@dharmaturtle
Copy link
Member

dharmaturtle commented Jun 20, 2023

What parameters should come pre-baked

Hmm I want to look into if there's a way create an attribute that just wraps a Gen (or AutoGenConfig), so people don't have to create an attribute for everything. Gimme a bit sorry; you're moving faster than I am 😅 I'm staring at the [<Property(AutoGenConfig = typeof<ConfigWithArgs>, AutoGenConfigArgs = [|"foo"; 13|])>] in the readme thinking "is that really the best you can do". I want to dig into this a little before making people (us or users) have dozens of little attributes.

Documentation

Hm, the readme is rather dense with many code blocks. It also uses F# specific segments like

Tests returning Async<Result<_,_>> or Task<Result<_,_>> are run synchronously and are expected to be in the Ok state.

I think it would be better to have a separate readme for C#, (depending on how far you're willing to go - if it's just the one summary/details then sure that's NBD)

@JohnEffo
Copy link
Contributor Author

What parameters should come pre-baked

I looked into trying to create a generic Range<'t> max min and came up short. Which made me think that if we add them in there were going to need to be lots, so if you can think of a way of doing it that would be great; although looking at my examples I almost think having lots of smaller well defined attributes may be easier to parse than more general purposes ones.

  [Property]
  public bool ResultOfAddingPositiveAndNegativeLessThanPositive(
    [Positive] int positive,
    [Negative] int negative)
    => positive + negative < positive;

vs

 [Property]
  public bool ResultOfAddingPositiveAndNegativeLessThanPositive(
    [Int32Range(1, Int32.MaxValue)] int positive,
    [Int32Range(Int32.MinValue, -1 )] int negative)
    => positive + negative < positive;

Documentation

I'll start on a separate C# documentation file. and put a

Go Here For C# Documentation

sized link at the top of the current readme

@dharmaturtle
Copy link
Member

From my own readme:

This works around the constraint that Attribute parameters must be a constant.

😭

It looks like C#11 supports generic attributes, though F# doesn't. So someday we might be able to do [<Property<ConfigWithArgs>(AutoGenConfigArgs = [|"foo"; 13|])>]. Back on topic,

I almost think having lots of smaller well defined attributes may be easier to parse than more general purposes ones.

💯

using Gen = Hedgehog.Linq.Gen;
using Range = Hedgehog.Linq.Range;

namespace csharp_attribute_based_parameters_comparision;
Copy link
Member

Choose a reason for hiding this comment

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

Could this name be more general? Might be useful for other C# examples. Or heck, could even turn it into a real test suite. I should probably add it to CICD anyway - doesn't really make sense to have broken examples.

Choose a reason for hiding this comment

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

Have updated this, I realised this when I came to do the documentation and wanted to put add the documentation examples.

src/Hedgehog.Xunit/Attributes.fs Outdated Show resolved Hide resolved
src/Hedgehog.Xunit/Attributes.fs Show resolved Hide resolved
@JohnEffo
Copy link
Contributor Author

A couple of things:

I Cannot run an async property:
[Property]
public async Task Async_with_exception_shrinks(int i)
{
await Task.Delay(100);
//Assert.True(i +i == 1);
return true;
}
System.Exception
*** Failed! Falsifiable (after 1 test):
[0]
System.ArgumentException: The type or method has 1 generic parameter(s), but 2 generic argument(s) were provided. A generic argument must be provided for each generic parameter.
at System.RuntimeType.SanityCheckGenericArguments(RuntimeType[] genericArguments, RuntimeType[] genericParameters)
at System.Reflection.RuntimeMethodInfo.MakeGenericMethod(Type[] methodInstantiation)
at InternalLogic.yieldAndCheckReturnValue(Object x) in C:\src\personal\fsharp-hedgehog-xunit\src\Hedgehog.Xunit\InternalLogic.fs:line 120
I have stepped through but could not figure it out.

Only the given contructors can be used for the attributes in C# this means that I can use autoGenConfigArgs this is not a problem for me as I'd always use, parameter based generators if I required to pass in argruments, but it does mean I can adjust the size via the property would you be happy for me to add:

new(tests, shrinks, size)
new(autoGenConfig:Type, tests, shrinks , size)
For Property and Properties. Other options would be to make the attributes mutable properties, but this would likely affect your current users.

@JohnEffo
Copy link
Contributor Author

A few more things.

I've committed the documentation, sans the Async docs
I've added some rules to F# doc to add a little more spacing.
I did this before copying it over for C#, if you don't like it just revert it.
I'll have a go at looking at the Async problem over the weekend but could do with your input on that.
Cheers
John

@JohnEffo
Copy link
Contributor Author

JohnEffo commented Jun 24, 2023

Commit 48b88d0 allows me to run the async properties below which were not running previously, this has meant making a change to the matching logic in InternnalLogin.yieldAndCheckReturnValue all the tests pass but I'd feel happier if you guys could take a look.

  public Task Fast()
  {
    return Task.FromResult(0);
  }

  [Property]
  public async Task Async_property_which_returns_task_can_run(
    int i)
  {
     await Fast();
     Assert.True(i == i);
  }

  [Property]
  public async Task<bool> Async_property_which_returns_boolean_task_can_run(bool i)
  {
    await Fast();
    return i || !i;
  }

@coveralls
Copy link

coveralls commented Jun 30, 2023

Pull Request Test Coverage Report for Build 5465920435

  • 24 of 25 (96.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.7%) to 90.857%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Hedgehog.Xunit/InternalLogic.fs 22 23 95.65%
Totals Coverage Status
Change from base Build 5431100222: 1.7%
Covered Lines: 211
Relevant Lines: 228

💛 - Coveralls

@dharmaturtle dharmaturtle force-pushed the nethod-parameter-generator branch 5 times, most recently from 1bec866 to 0b1197c Compare June 30, 2023 23:03
@dharmaturtle dharmaturtle mentioned this pull request Jul 1, 2023
@dharmaturtle dharmaturtle force-pushed the nethod-parameter-generator branch 2 times, most recently from 036a381 to f51725e Compare July 1, 2023 23:56
@dharmaturtle dharmaturtle force-pushed the nethod-parameter-generator branch 4 times, most recently from 1c4d20a to cafec87 Compare July 3, 2023 21:36
@dharmaturtle
Copy link
Member

@JohnEffo Okay I'm more or less done with my changes - would you like review them? My biggest logic change was to drop the genType.ContainsGenericParameters conditional branch and adding .First() to GetGenericArguments(). It seems like Task in C# yields two generic arguments, but not in F#. No idea why 🤷‍♂️

I also made many changes to the readme. I noticed that you used --- in a few places - I swapped those out for emojis because I don't like the way --- looks, but do agree that some section headers need to be emphasized. Hopefully the color in the emojis makes them pop out more? I removed some of the async/task documentation because that's the way it should work - I beleive "If the test returns Async<_> or Task<_>, then Async.RunSynchronously is called" encompasses that code. I also reduced the "justification" for GenAttribute, because I felt like that's not the place for API documentation. It could go in Tips - though I expanded the example there to hopefully make it more obvious the value GenAttribute provides. Let me know what you think, or if I should just :shipit: already 🤪

@JohnEffo
Copy link
Contributor Author

JohnEffo commented Jul 4, 2023

I felt cheeky changing your readme in any case and your changes look good. Thank you for time you spent going through the C# examples. I tried getting something like the code you added below to work, so having a working example is really useful.

 [Property(AutoGenConfig = typeof(ConfigWithArgs), AutoGenConfigArgs = new object[] { "foo", 13 })]
 public bool This_also_passes(string s, int i) =>
     s == "foo" && i == 13;

I'm happy for you to merge.

@dharmaturtle dharmaturtle merged commit 69abdd7 into hedgehogqa:main Jul 5, 2023
1 check passed
@dharmaturtle
Copy link
Member

Okay this is out with 0.6.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants