Release 4.6.0 #151
Release 4.6.0 #151
Conversation
Defensive code to prevent nether border if it doesn't need one.
Fix Nether WorldBorder
There was a problem hiding this comment.
Pull request overview
This PR upgrades the project to release version 4.6.0, including a major migration from JUnit 4 + PowerMock to JUnit 5 + Mockito, updating Java from version 17 to 21, and switching from Spigot API to Paper API.
Key Changes:
- Migration from JUnit 4 + PowerMock to JUnit 5 + Mockito with static mocking support
- Java version upgrade from 17 to 21
- Dependency updates: Paper API 1.21.10, BentoBox 3.10.0, JUnit 5.10.2, Mockito 5.11.0, MockBukkit v1.21-SNAPSHOT
- Introduction of CommonTestSetup base class to centralize test infrastructure and reduce duplication
- Fix for nether world border display logic in ShowVirtualWorldBorder
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updates Java to 21, replaces PowerMock with JUnit 5 and Mockito, switches from Spigot to Paper API, updates Maven plugin versions |
| .github/workflows/build.yml | Updates CI workflow to use JDK 21 instead of JDK 17 |
| CommonTestSetup.java | New base class providing common test setup including mocks for BentoBox, Bukkit, players, islands, and world management |
| WhiteBox.java | New utility class for setting private static fields via reflection (replaces PowerMock's Whitebox) |
| TestWorldSettings.java | New test implementation of WorldSettings interface for test configuration |
| ShowVirtualWorldBorderTest.java | Migrates from JUnit 4/PowerMock to JUnit 5/Mockito, extends CommonTestSetup |
| ShowBarrierTest.java | Migrates from JUnit 4/PowerMock to JUnit 5/Mockito, extends CommonTestSetup |
| PlayerListenerTest.java | Migrates from JUnit 4/PowerMock to JUnit 5/Mockito, extends CommonTestSetup, updates PlayerRespawnEvent constructor |
| IslandBorderCommandTest.java | Migrates from JUnit 4/PowerMock to JUnit 5/Mockito, extends CommonTestSetup |
| BorderTypeCommandTest.java | Migrates from JUnit 4/PowerMock to JUnit 5/Mockito, extends CommonTestSetup, fixes test assertion |
| SettingsTest.java | Simple migration from JUnit 4 to JUnit 5 annotations |
| ShowVirtualWorldBorder.java | Adds check for non-island nether worlds, changes location handling to use player's world coordinate space |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (player.getWorld().getEnvironment() == Environment.NETHER && !addon.getPlugin().getIWM().isIslandNether(player.getWorld())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This new check prevents showing borders in non-island nether worlds, but there are no test cases in ShowVirtualWorldBorderTest.java that verify this behavior. Tests should be added to cover both the case where the player is in an island nether world (border should show) and a non-island nether world (border should not show).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
src/main/java/world/bentobox/border/listeners/ShowVirtualWorldBorder.java
Show resolved
Hide resolved
| MockedStatic<User> mockedUser = Mockito.mockStatic(User.class, Mockito.RETURNS_MOCKS); | ||
| mockedUser.when(() -> User.getInstance(any(Player.class))).thenReturn(user); |
There was a problem hiding this comment.
A new MockedStatic instance is created here but never closed. This will cause a resource leak and can lead to test pollution where this mock affects subsequent tests. The mockedUser variable should be stored as a field and explicitly closed in the tearDown method.
| MockedStatic<User> mockedUser = Mockito.mockStatic(User.class, Mockito.RETURNS_MOCKS); | ||
| mockedUser.when(() -> User.getInstance(any(Player.class))).thenReturn(user); |
There was a problem hiding this comment.
A new MockedStatic instance is created here but never closed. This will cause a resource leak and can lead to test pollution where this mock affects subsequent tests. The mockedUser variable should be stored as a field and explicitly closed in the tearDown method.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| MockedStatic<User> mockedUser = Mockito.mockStatic(User.class, Mockito.RETURNS_MOCKS); | ||
| mockedUser.when(() -> User.getInstance(any(Player.class))).thenReturn(user); |
There was a problem hiding this comment.
A new MockedStatic instance is created here but never closed. This will cause a resource leak and can lead to test pollution where this mock affects subsequent tests. The mockedUser variable should be stored as a field and explicitly closed in the tearDown method.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@tastybento I've opened a new pull request, #152, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #153, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <[email protected]>
|
@tastybento I've opened a new pull request, #154, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
No description provided.