Skip to content

AVRO-2702: ResolvingGrammarGenerator Union use reader instead of writer schema#980

Closed
bellemare wants to merge 1 commit intoapache:masterfrom
bellemare:AVRO-2702
Closed

AVRO-2702: ResolvingGrammarGenerator Union use reader instead of writer schema#980
bellemare wants to merge 1 commit intoapache:masterfrom
bellemare:AVRO-2702

Conversation

@bellemare
Copy link
Contributor

@bellemare bellemare commented Nov 9, 2020

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

SEE FOLLOWING COMMENT - The code that was changed does not appear to be testing thoroughly as is, and it may require more extensive unit test changes than a simple one-off.

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@github-actions github-actions bot added the Java Pull Requests for Java binding label Nov 9, 2020
@bellemare
Copy link
Contributor Author

One of the major issues with this PR is that there doesn't appear to be any existing tests that test for it. The reporter issued a comprehensive test (in Kotlin) that does reveal if the patch works (I have verified the code to indeed work), but it seems heavy-handed to implement a single test for what clearly seemed like a mistake in the union coding.

I am interested to see what a committer says about what we should do with regards to testing this, as it could be symptomatic of underlying issues with the ResolvingGrammarGenerator tests, particularly around nesting and strings.

Alternatively, I am fine with porting the single-purpose test from Kotlin to Java and submitting that.

@RyanSkraba
Copy link
Contributor

Hello! This looks important, and I'd like to get a 1.10.1 release candidate soon.

I don't think anyone is happy with missing unit tests, but this does look like the obvious fix. Would it be possible to include the ported Bar.kt test for this specific bug? At the very least, having it in place could encourage other tests to spring up...

Thanks for your contribution!

@RyanSkraba RyanSkraba changed the title ResolvingGrammarGenerator Union use reader instead of writer schema AVRO-2702: ResolvingGrammarGenerator Union use reader instead of writer schema Nov 16, 2020
@RyanSkraba
Copy link
Contributor

Included in #987 and cherry-picked into branch-1.10. I added the unit test translated from Kotlin. Thanks for your contribution!

@RyanSkraba RyanSkraba closed this Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java Pull Requests for Java binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants