Skip to content

Conversation

@XN137
Copy link
Contributor

@XN137 XN137 commented Jul 6, 2022

  • upgrade junit5 to 5.8.2
  • upgrade assertj to 3.23.1
  • upgrade mockserver to 5.13.2
  • upgrade mockito to 4.6.1
  • org.mockito.Matchers class was removed

@XN137 XN137 force-pushed the upgrade-test-deps branch from 52d0eb6 to 80beb6f Compare July 6, 2022 15:48
@XN137
Copy link
Contributor Author

XN137 commented Jul 6, 2022

rebased due to merge conflict

@szehon-ho
Copy link
Member

It makes sense to upgrade to me, but do you think we can leave out the ArgumentMatchers => Mockito refactor? It doesn't seem that valuable unless its enforced going forward, one way or the other. As any() is defined on ArgumentMatchers, I was actually thinking it makes sense to call it from where it is defined.

@XN137
Copy link
Contributor Author

XN137 commented Jul 8, 2022

As any() is defined on ArgumentMatchers, I was actually thinking it makes sense to call it from where it is defined.

this was the situation on master:

~/code/iceberg$ rg "Mockito.any" | wc -l
45
~/code/iceberg$ rg "ArgumentMatchers.any" | wc -l
9

so it looked like to me, that the any-methods from Mockito might be preferred across the codebase.
thus i replaced the now deleted Matchers with Mockito
and refactored ArgumentMatchers usage as well for consistency.

i think generally it is not unusual to use wildcard imports with Mockito as an exception:
import static org.mockito.Mockito.*;
or just import Mockito and have all related methods discoverable from a single entry point (i.e. auto-complete).

that being said, i am fine to revert this part of the change if you still want me to?

@szehon-ho
Copy link
Member

Yea , I do understand the desire to make it the same, but there's definitely many correct ways unless there is some definite rule, so I'd say let's revert those changes to focus this pr? Other changes look good to me.

- upgrade junit5 to 5.8.2
- upgrade assertj to 3.23.1
- upgrade mockserver to 5.13.2
- upgrade mockito to 4.6.1
- org.mockito.Matchers class was removed
@XN137 XN137 force-pushed the upgrade-test-deps branch from 80beb6f to 17b66d0 Compare July 9, 2022 08:15
@github-actions github-actions bot removed the ALIYUN label Jul 9, 2022
@XN137
Copy link
Contributor Author

XN137 commented Jul 9, 2022

@szehon-ho ok i have removed the refactoring around ArgumentMatchers now

@szehon-ho
Copy link
Member

Missed the message, thanks, looks good to me. Will wait a little bit to see any other comments, otherwise will merge soon.

@szehon-ho szehon-ho merged commit 52aa642 into apache:master Jul 13, 2022
@szehon-ho
Copy link
Member

Merged, thanks @XN137 for change and @nastra for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants