Skip to content

[BEAM-3271] Add TestStream translation#3283

Merged
asfgit merged 4 commits into
apache:masterfrom
kennknowles:TestStream-translation
Jun 5, 2017
Merged

[BEAM-3271] Add TestStream translation#3283
asfgit merged 4 commits into
apache:masterfrom
kennknowles:TestStream-translation

Conversation

@kennknowles
Copy link
Copy Markdown
Member

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.
  • 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.

R: @tgroh

Because this is a real primitive, I added a somewhat comprehensive payload and tested it a bit.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.03%) to 70.648% when pulling 3bf6fc3 on kennknowles:TestStream-translation into 217f085 on apache:master.

@Override
public RunnerApi.FunctionSpec translate(
AppliedPTransform<?, ?, TestStream<?>> transform, SdkComponents components) {
return RunnerApi.FunctionSpec.newBuilder().setUrn(getUrn(transform.getTransform())).build();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems like it's missing the payload

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

True! Added an additional test going through the registrar.

@kennknowles kennknowles force-pushed the TestStream-translation branch from 3bf6fc3 to 0cedc61 Compare June 2, 2017 17:07
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.009%) to 70.68% when pulling 0cedc61 on kennknowles:TestStream-translation into f1386c1 on apache:master.

Copy link
Copy Markdown
Member

@tgroh tgroh left a comment

Choose a reason for hiding this comment

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

LGTM

import org.junit.runners.Suite;

/** Tests for {@link TestStreamTranslation}. */
@RunWith(Suite.class)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do this via suite instead of Parameterized directly? Just future-proofing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Somehow a top-level Parameterized bothers me. Two test methods sharing a set of parameters to exhaustively loop over is a pretty special case so I didn't naturally make it the default.

@asfgit asfgit merged commit 0cedc61 into apache:master Jun 5, 2017
asfgit pushed a commit that referenced this pull request Jun 5, 2017
  Add TestStream translation
  Expose internal constructors for TestStream events
  Add TestStreamPayload to Runner API proto
  Add translation convenience for non-composites
@kennknowles kennknowles deleted the TestStream-translation branch September 29, 2017 21:25
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