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

Records cannot be customised #87

Closed
jk-idealo opened this issue Mar 9, 2023 · 4 comments
Closed

Records cannot be customised #87

jk-idealo opened this issue Mar 9, 2023 · 4 comments

Comments

@jk-idealo
Copy link
Collaborator

jk-idealo commented Mar 9, 2023

I remember that despite Fixture being compiled in Java 11, we (or rather one of my amazing co-authors) once enabled it to successfully create records when used in a Java 17 application.

Now, in Java 17, I find myself unable to customise the behaviour:

var actual = fixture().build(MyRecord.class).with("id","hello").create();
assertThat(actual.id()).isEqualTo("hello");

This test is red, as id will still contain a random string value.

Now, the bonus challenge is: How do we test records in an environment (Java 11) that doesn't provide records?

@akutschera
Copy link
Collaborator

In order for this to work, we would need access to the "id":"hello"-mapping when we use a constructor for an object.
The InstanceFactory needs to know not only the paramter type, but also the parameter name, instead of iterating over constructor.getGenericParameterTypes() we should use constructor.getParameters() where we have both the name("id") and the type ("String"). That should be enough to generate a record with fixed values.

@akutschera
Copy link
Collaborator

While trying to test this I found that we can do this for records, but not for "normal" classes.
The reason for this is that by default the java compiler does not include parameter names in the class file (see the Oracle docs).
That means that the code above will work with

public record MyRecord(String id) {}

but not with

public class MyRecord {
  String id;
  public MyRecord(String id) {
    this.id = id;
  }
}

The constructor parameter will show up as arg0 when we access it via reflection. You could get around this when you compile your code with the -parameters compiler option but doing that just for testing with JavaFixture seems a bit strange.
Changing the test to use with("arg0","hello") will not work because JavaFixture checks if all the fields you specify in the with-part actually exist in the class. We do this to prevent typos in field names failing your tests.

I propose the following behaviour:

  • we add the customization to the constructor parameters, because that is the only way we can control records
  • we wait until somebody complains that they cannot customize an object via constructor parameter because of the above reasons, because I think it will take a while before you try to fixture a class for which you cannot set the fields via reflection and therefore fall back to using a random constructor.
  • we will keep the Fixture.construct(Some.class) method as is, i.e. you cannot customize that.

@Nylle
Copy link
Owner

Nylle commented Mar 31, 2023

It's a bummer that "normal" classes use generic constructor-parameter names. On the other hand, I'm also a little bit glad about this, because otherwise it would seem very very magical, at least as long those parameter-names can even change depending on how you compile your code (as you explained to me the other day).
Using custom compiler-options are a no-go for me, I agree with you fully.
The way Fixture checks field-names for typos is exactly how we want it (I think it was even your proposal), because classes without setters can only be customised via field-name reflection. As you pointed out earlier, this is not refactoring-safe which makes this check necessary as to not silently fail and cause unexpected outcomes. So I agree as well.
As to your proposal:

  • we add the customization to the constructor parameters, because that is the only way we can control records

I think this is the best way to go for now.

  • we wait until somebody complains that they cannot customize an object via constructor parameter because of the above reasons

Pragmatism is always a good way to go. In case we run into trouble, I can see another possible option to overcome this, albeit a potentially invasive one. So let's see if we really need it.

  • we will keep the Fixture.construct(Some.class) method as is, i.e. you cannot customize that.

I agree.

I think we're in sync. Good work!

akutschera added a commit to akutschera/JavaFixture that referenced this issue Apr 11, 2023
Nylle pushed a commit that referenced this issue Apr 12, 2023
@Nylle
Copy link
Owner

Nylle commented May 1, 2023

Fixed in version 2.9.6.

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

No branches or pull requests

3 participants