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

fix: fixes two issues in the generation of mocks. #3120

Conversation

TizianoCoroneo
Copy link
Contributor

@TizianoCoroneo TizianoCoroneo commented Jul 13, 2023

Hello 👋

I found and fixed a couple issues with the generation of mocks.

Screenshot 2023-07-13 at 11 26 06

  1. when generating properties with conflicting names in the Mock extensions, the setter always uses a _set function that is undefined. I changed it to use the appropriate _set{List, Scalar, ...} function.
  2. when generating the convenience init for Mock objects, if the mock requires a list of custom scalars the current implementation just tries to use the existing _setList function that requires the elements of the list to conform to MockFieldValue. Since scalars do not conform to MockField, this doesn't compile in the client app. I fixed it by adding two overloads: one to the _setList function that takes an array of AnyScalarType instead, and one to subscript.

Please double check my logic, I'm not very familiar with how this new Mock system works.

I also updated the tests to reflect these changes. I noticed that the ApolloCodegenLib does not test the result of the code generation; it would be very nice to have a "two steps" setup where first we test the generation of the code itself, as today's tests do, and then compile the result of the code generation and assert that the compilation is successful.
A similar setup would have caught both these issues. Just a suggestion 😄

@netlify
Copy link

netlify bot commented Jul 13, 2023

👷 Deploy request for apollo-ios-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit f223a1e

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

These are some really great fixes! Thanks so much for this @TizianoCoroneo!


As for the suggestion about code gen tests, we have been talking about that internally for a bit. We do have a plan to implement more of this, but we need to finish building out the infrastructure to make it work.

Right now, there is a folder Tests/CodegenIntegrationTests. Inside that folder, we want to start highlighting issues like this one and documenting test cases to prevent regressions on them in the future.

The idea is to have a schema file, at least one operation, and a README that explains the thing we are trying to test for now. When we build out the infrastructure, we're going to write an actual test that runs codegen over the schema and operations; builds and compiles an example project; and runs a unit test or two that consumes the generated models to ensure they actually work properly.

If you're willing to add it to this PR, you could create a test case in that folder for us. Just a small example schema, operation, and README. I'll write the actual tests when we get the infrastructure set up.

}
}

public func _setList<T: AnyScalarType & Hashable>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename this ti setScalarList. The point of defining these methods in the first place was to avoid the issues we were having with trying to make sure the compiler always used the right overload for the functions based on the types. Just to maintain that pattern, they should all have unique names.

@AnthonyMDev
Copy link
Contributor

I'd like to get these fixes in ASAP! @TizianoCoroneo would you like to make the suggested change of the function name to setScalarList or would you like me to pull your branch and make the changes myself before merging?

@TizianoCoroneo
Copy link
Contributor Author

I'd like to get these fixes in ASAP! @TizianoCoroneo would you like to make the suggested change of the function name to setScalarList or would you like me to pull your branch and make the changes myself before merging?

Done, let me know if it looks good. I also added a test schema to the codegen tests folder

Copy link
Member

@BobaFetters BobaFetters left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for doing this!

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Thanks so much for this @TizianoCoroneo! Looks great. And the added integration test info is going to be super helpful once we get that infra set up.

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.

3 participants