You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/01/06 01:47:08 UTC
[GitHub] [beam] anantdamle opened a new pull request #13673: [BEAM-11572] Replace Assert.asserThat with MacherAssert.assertThat across entire codebase
anantdamle opened a new pull request #13673:
URL: https://github.com/apache/beam/pull/13673
JUnit [`Assert#assertThat`](https://junit.org/junit4/javadoc/latest/org/junit/Assert.html#assertThat(T,%20org.hamcrest.Matcher)) has is deprecated and needs to be replaced across entire codebase with [`MatcherAssert#assertThat`](https://junit.org/junit4/javadoc/latest/org/junit/Assert.html#assertThat(T,%20org.hamcrest.Matcher)) as per recommendation.
@iemejia Thanks for reviewing my previous PRs. This is a massive PR, but few line changes in each file. how should I proceed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] anantdamle commented on a change in pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
anantdamle commented on a change in pull request #13673:
URL: https://github.com/apache/beam/pull/13673#discussion_r552426243
##########
File path: sdks/java/io/xml/src/test/java/org/apache/beam/sdk/io/xml/XmlIOTest.java
##########
@@ -19,6 +19,7 @@
import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasDisplayItem;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matcherorg.hamcrest.MatcherAssert.assertThat;
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia edited a comment on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
iemejia edited a comment on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755179604
Seems there is another error in the nexmark module. I will let you and @ibzib follow with this review (that will give me some extra time to finish other stuff). Thanks.
```
09:11:50 > Task :sdks:java:testing:nexmark:checkstyleMain
09:11:50 [ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_Commit@2/src/sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/queries/NexmarkQueryModel.java:102: Using deprecated Assert.assertThat. Please use MatcherAssert.assertThat. [ForbidDeprecatedAssertThat]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] anantdamle commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
anantdamle commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755099575
Thanks let me do that.
So your recommendation is to make it part of the linter, so that slowly all code will get updated with individual PRs or in addition to this one time large one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755079304
> Can you guide me on how to add that check?
You can add it to our [checkstyle.xml](https://github.com/apache/beam/blob/master/sdks/java/build-tools/src/main/resources/beam/checkstyle.xml) config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755453479
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib merged pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
ibzib merged pull request #13673:
URL: https://github.com/apache/beam/pull/13673
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MacherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755073583
@anantdamle Do you think we could add this to our linters to prevent future usage of the deprecated class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] iemejia commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
iemejia commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755179604
Seems there is another error in the nexmark module. I will let you and @ibzib follow with this review (that will give me some extra time to finish other stuff). Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] anantdamle commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
anantdamle commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755212304
> Seems there is another error in the nexmark module. I will let you and @ibzib follow with this review (that will give me some extra time to finish other stuff). Thanks.
>
> ```
> 09:11:50 > Task :sdks:java:testing:nexmark:checkstyleMain
> 09:11:50 [ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_Commit@2/src/sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/queries/NexmarkQueryModel.java:102: Using deprecated Assert.assertThat. Please use MatcherAssert.assertThat. [ForbidDeprecatedAssertThat]
> ```
Thanks @iemejia fixed that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] anantdamle commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
anantdamle commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755152262
Added linter rule. To my surprise, found one usage!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #13673: [BEAM-11572] Replace Assert.asserThat with MacherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #13673:
URL: https://github.com/apache/beam/pull/13673#discussion_r552364605
##########
File path: sdks/java/io/xml/src/test/java/org/apache/beam/sdk/io/xml/XmlIOTest.java
##########
@@ -19,6 +19,7 @@
import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasDisplayItem;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matcherorg.hamcrest.MatcherAssert.assertThat;
Review comment:
There's a typo here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] anantdamle commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MacherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
anantdamle commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755075733
Yes, that should be the case.
Can you guide me on how to add that check?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] anantdamle commented on pull request #13673: [BEAM-11572] Replace Assert.asserThat with MatcherAssert.assertThat across entire codebase
Posted by GitBox <gi...@apache.org>.
anantdamle commented on pull request #13673:
URL: https://github.com/apache/beam/pull/13673#issuecomment-755267036
@ibzib there is some test failure [org.apache.beam.sdk.transforms.ParDoLifecycleTest.testTeardownCalledAfterExceptionInProcessElement](https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/15391/testReport/junit/org.apache.beam.sdk.transforms/ParDoLifecycleTest/testTeardownCalledAfterExceptionInProcessElement/)
I am unable to think of a reason for this failure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org