-
Notifications
You must be signed in to change notification settings - Fork 1.7k
AVRO-3689: [lang/java/avro] Fix flaky test ‘testAnnotationMultiAvroMeta’ #2012
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
base: main
Are you sure you want to change the base?
Conversation
| char[] expectedArrays = expectedString.toCharArray(); | ||
| Arrays.sort(schmArrays); | ||
| Arrays.sort(expectedArrays); | ||
| assertEquals(new String(schmArrays), new String(expectedArrays)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done, may be it should be nice to add nondex plugin in build.
For the fix, i would prefer standard schema comparison, like
Field field = new Field("a", Schema.create(Schema.Type.INT));
field.addProp("L", "W");
field.addProp("K", "V");
Schema avroMultiMeta = Schema.createRecord("RAvroMultiMeta", null, "org.apache.avro.reflect.TestReflect", false,
Arrays.asList(field));
avroMultiMeta.addProp("X", "Y");
avroMultiMeta.addProp("A", "B");
Schema schema = ReflectData.get().getSchema(RAvroMultiMeta.class);
assertEquals(avroMultiMeta, schema);(In same time, it test Schema.equals method).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback on my pull request. In terms of the fix, I think using a standard schema comparison tool like the one you suggested would be a good idea. I will make the necessary changes and update the pull request accordingly. Thanks again for your suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I agree that adding a nondex plugin to the build would be a good idea. Using nondex can improve the reliability and stability of the code, which can help developers ensure that code changes don't result in unexpected outcomes. I will submit a new pull request about adding nondex to Avro for review.
Here is the link to Nondex: https://github.com/TestingResearchIllinois/NonDex. The lastest version can support Java 9+ project.
clesaec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THanks for nondex reference.
LGTM,
advice to one avro commiter for merging.
What is the purpose of the change
This pull request fixes the flaky test 'testAnnotationMultiAvroMeta', which was causing issues with AVRO-3689.
Verifying this change
Run this maven command to reproduce the issue:
Documentation