Skip to content

Test all Known Coders to ensure they Serialize via URN#2517

Closed
tgroh wants to merge 2 commits intoapache:masterfrom
tgroh:coders_fixes
Closed

Test all Known Coders to ensure they Serialize via URN#2517
tgroh wants to merge 2 commits intoapache:masterfrom
tgroh:coders_fixes

Conversation

@tgroh
Copy link
Member

@tgroh tgroh commented Apr 12, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

Fix Coders to not encode VarIntCoders as VarLongCoders, and
VarLongCoders not as known coders.

Ensure that all known coders are tested.

Use getComponents rather than getCoderArguments for known coders.

Fix Coders to not encode VarIntCoders as VarLongCoders, and
VarLongCoders not as known coders.

Ensure that all known coders are tested.

Use getComponents rather than getCoderArguments for known coders.
@tgroh
Copy link
Member Author

tgroh commented Apr 12, 2017

R: @lukecwik

Copy link
Member

@lukecwik lukecwik left a comment

Choose a reason for hiding this comment

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

Minor comments in test and then LGTM

.build();

@BeforeClass
public static void validateKnownCoders() {
Copy link
Member

Choose a reason for hiding this comment

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

Use the Enclosed test runner and have two static nested classes, one containing this test and one containing the parameterized test. This way validateKnownCoders will not be incorrectly reported as a test error vs a test failure.
See:
http://stackoverflow.com/questions/8758294/test-cases-in-inner-classes-with-junit
or AvroIOTransformTest.java as well

@tgroh
Copy link
Member Author

tgroh commented Apr 13, 2017

Done.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 70.082% when pulling 42f5497 on tgroh:coders_fixes into dc672f4 on apache:master.

@asfbot
Copy link

asfbot commented Apr 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9480/
--none--

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 70.096% when pulling 42f5497 on tgroh:coders_fixes into dc672f4 on apache:master.

@asfbot
Copy link

asfbot commented Apr 13, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9484/
--none--

@lukecwik
Copy link
Member

LGTM

@asfgit asfgit closed this in 4c4fdf2 Apr 13, 2017
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.

4 participants