-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-3659] Port UserScoreTest off DoFnTester #7126
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
[BEAM-3659] Port UserScoreTest off DoFnTester #7126
Conversation
aromanenko-dev
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! It looks almost ok for me, I just added a couple of minor notes.
examples/java/src/test/java/org/apache/beam/examples/complete/game/UserScoreTest.java
Show resolved
Hide resolved
| @Test | ||
| public void testParseEventFn() throws Exception { | ||
| DoFnTester<String, GameActionInfo> parseEventFn = DoFnTester.of(new ParseEventFn()); | ||
| PCollection<String> input = p.apply(Create.of(GAME_EVENTS).withCoder(StringUtf8Coder.of())); |
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.
I don't think we need to add explicitly default coder StringUtf8Coder
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.
I originally had it because the other tests were also using it. I've removed the coder from the other examples to stay consistent.
examples/java/src/main/java/org/apache/beam/examples/complete/game/UserScore.java
Show resolved
Hide resolved
examples/java/src/main/java/org/apache/beam/examples/complete/game/UserScore.java
Outdated
Show resolved
Hide resolved
c4a7a19 to
8136ca1
Compare
examples/java/src/test/java/org/apache/beam/examples/complete/game/UserScoreTest.java
Show resolved
Hide resolved
8136ca1 to
3e765fe
Compare
|
@kennknowles @aromanenko-dev I've addressed the comments, however, it seems that the java pre-commit is failing due to some GRPC test. Is this a known issue? Should I just trigger another build? |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
|
I'm not sure that pushes to |
3e765fe to
4ca0e89
Compare
|
thanks @kennknowles that worked. 🎉 @aromanenko-dev could you please review again? |
|
I'm merging since, finally, the changes were approved. |
Ports UserScoreTest off the
DoFnTesterin favor of testing through a real pipeline. This is subtask of the story of BEAM-3650 Deprecating and removing the DoFnTester.Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.It will help us expedite review of your Pull Request if you tag someone (e.g.
@username) to look at it.Post-Commit Tests Status (on master branch)