Skip to content
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

Get rid of powermock #16912

Closed
tisonkun opened this issue Aug 2, 2022 · 5 comments · Fixed by #17696
Closed

Get rid of powermock #16912

tisonkun opened this issue Aug 2, 2022 · 5 comments · Fixed by #17696

Comments

@tisonkun
Copy link
Member

tisonkun commented Aug 2, 2022

In this comment #16884 (comment) I wrote:

I don't like powermock as it lacks maintenance. It seems all usage of powermock is Whitebox. We can get rid of it with simple reflections. This can be a separate issue, though.

Thus I file this issue to investigate whether we can remove powermock from our testing framework.

WDTY @merlimat @lhotari @codelipenghui @nicoloboschi?

@shoothzj
Copy link
Member

shoothzj commented Aug 2, 2022

I think we can simply modify the access modifiers, and mark @VisableForTesting .We have used this in many codes.

@tisonkun
Copy link
Member Author

tisonkun commented Aug 2, 2022

@shoothzj IIUC do you suggest a replacement of powermock here?

@shoothzj
Copy link
Member

shoothzj commented Aug 2, 2022

@tisonkun Yes, I suggest to replace powermock. Using @VisableForTesting instead of reflections

@tisonkun
Copy link
Member Author

tisonkun commented Aug 2, 2022

@shoothzj thank you. I'll consider this approach when protptyping. There're some cases we try to hijack third-party lib's field, like eventLoopGroup in bookkeeper code. In those situations, we should still use reflections. (... or patching upstream, or change testing logics)

But yes, we can use multiple alternatives properly in each concrete case.

@tisonkun tisonkun mentioned this issue Aug 8, 2022
1 task
@tisonkun
Copy link
Member Author

tisonkun commented Aug 9, 2022

I draft an incompleted patch on #16997.

However, I notice that there's some code we hack in "set final" field, like:

EntryCache entryCache = spy((EntryCache) Whitebox.getInternalState(ledger, "entryCache"));
Whitebox.setInternalState(ledger, "entryCache", entryCache);

.. or even "set static final" field, like:

Whitebox.setInternalState(DirectMemoryUtils.class, "JVM_MAX_DIRECT_MEMORY", 1024L);

It seems powermock-reflect is the best library existing to handle these cases since JDK12+ it's banned changing final field instead you have to use Unsafe. I tend not to maintain such unsafe hacks within Pulsar codebase. And perhaps we should gradually refactor code to avoid depending on such manners.

cc @shoothzj @eolivelli @nicoloboschi

shoothzj pushed a commit that referenced this issue Aug 22, 2022
shoothzj pushed a commit that referenced this issue Sep 2, 2022
nodece pushed a commit to nodece/pulsar that referenced this issue Sep 8, 2022
shoothzj pushed a commit that referenced this issue Sep 10, 2022
Master Issue: #16912

Remove `WhiteBox` usage in `pulsar-client` and `pulsar-client-tools`. We then have only usages in `pulsar-broker`, `pulsar-io-elastic-search`, and `testmocks`.

cc @nicoloboschi @lhotari @shoothzj @eolivelli 

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed` 
(Please explain why)

- [ ] `doc` 
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)
shoothzj pushed a commit that referenced this issue Sep 12, 2022
Master Issue: #16912 

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed` 
(Please explain why)

- [ ] `doc` 
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)
tisonkun added a commit to tisonkun/pulsar that referenced this issue Sep 14, 2022
)

Master Issue: apache#16912 

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed` 
(Please explain why)

- [ ] `doc` 
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)
nicoloboschi pushed a commit to datastax/pulsar that referenced this issue Nov 23, 2022
)

Master Issue: apache#16912

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
(Please explain why)

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)

(cherry picked from commit adb63d8)
liangyepianzhou pushed a commit that referenced this issue Dec 14, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this issue Jan 10, 2023
…ache#17022)

Master Issue: apache#16912

- [x] `doc-not-needed`

cc @shoothzj @eolivelli @nicoloboschi @Technoboy-

Signed-off-by: tison <[email protected]>
(cherry picked from commit 7747795)
(cherry picked from commit 35097fd)
nicoloboschi pushed a commit to datastax/pulsar that referenced this issue Jan 11, 2023
…ache#17022)

Master Issue: apache#16912

- [x] `doc-not-needed`

cc @shoothzj @eolivelli @nicoloboschi @Technoboy-

Signed-off-by: tison <[email protected]>
(cherry picked from commit 7747795)
(cherry picked from commit 35097fd)
Technoboy- pushed a commit that referenced this issue Feb 8, 2023
Master Issue: #16912 

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed` 
(Please explain why)

- [ ] `doc` 
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants