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 introspection of list field in schema #5326

Merged
merged 2 commits into from Aug 21, 2017

Conversation

limdauto
Copy link
Contributor

@limdauto limdauto commented Aug 12, 2017

I hope the commit is self-explanatory. Please let me know if you need more information.

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 13, 2017

@limdauto I think we need a test case to cover the intended behaviour here. (Not least because we want to avoid regressions as the schema generation code grows/matures.)

@limdauto
Copy link
Contributor Author

limdauto commented Aug 17, 2017

@carltongibson hi thanks for the feedback I have just realized that this fixes a known issue: #5311 i will add a test case this weekend.

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 17, 2017

@limdauto Awesome. Thanks!

@limdauto
Copy link
Contributor Author

limdauto commented Aug 20, 2017

@carltongibson sorry I was moving house last week so only got a chance to do it now. I have added a test.

As a general question, the test suite for schema generation seems to be all integration test, i.e. we need to make a call to the API and get back the schema to check for validity. So it takes a bit of extra effort to test smaller fixes like this. What's your opinion of introducing more unit tests to some lower-level methods?

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 21, 2017

What's your opinion of introducing more unit tests to some lower-level methods?

I all for that!

Aside: this is all still new. First batch of tests are deliberately high-level: "Does it all work?". As it matures, yes, we need to move to more targeted testing.

@carltongibson carltongibson added this to the 3.6.4 Release milestone Aug 21, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

Lovely. Thank you very much!

@carltongibson carltongibson merged commit d2286ba into encode:master Aug 21, 2017
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants