-
-
Notifications
You must be signed in to change notification settings - Fork 21
GH-1228 Litegration #1228
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
base: master
Are you sure you want to change the base?
GH-1228 Litegration #1228
Conversation
|
fajne czy nie fajne? |
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.
Code Review
This pull request introduces Litegration for running integration tests on a Paper server, which is a great step towards improving code quality and reliability. The changes include setting up the build files, adding a base test class, and an initial integration test for the CatboyService.
My review includes suggestions to improve the build script configuration by removing redundancies and hardcoded versions, and to enhance the readability and maintainability of the new test code. I've also raised a high-severity concern about disabling parallel execution in Gradle, which could impact build performance.
| # https://docs.gradle.org/current/userguide/build_environment.html#sec:gradle_configuration_properties | ||
|
|
||
| org.gradle.parallel=true | ||
| org.gradle.parallel=false |
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.
Disabling parallel execution can significantly increase build times. While this might be a necessary workaround for the integration tests to function correctly, the reason for this change should be documented with a comment. This will help other developers understand the limitation and potentially re-enable it in the future if the underlying issue is resolved. The same applies to org.gradle.configuration-cache.parallel on line 6.
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.
@Rollczi Can't we toggle this off per-task?
| repositories { | ||
| maven("https://repo.eternalcode.pl/releases/") | ||
| } |
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.
|
|
||
| tasks.testPaper { | ||
| eula = true | ||
| serverVersion = "1.21.7" |
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.
The server version 1.21.7 is hardcoded. To maintain consistency and ease of updates, it's better to define this version in your buildSrc/src/main/kotlin/Versions.kt file and reference it here.
For example, you could add const val PAPER_TEST_SERVER = "1.21.7" to Versions.kt and then use Versions.PAPER_TEST_SERVER here.
|
|
||
| testPaperImplementation("dev.rollczi:litegration-client-mcprotocollib:${Versions.LITEGRATION}") | ||
|
|
||
| testPaperImplementation(platform("org.junit:junit-bom:6.0.0")) |
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.
| testPaperImplementation(platform("org.junit:junit-bom:6.0.0")) | ||
| testPaperImplementation("org.spigotmc:spigot-api:${Versions.SPIGOT_API}") | ||
|
|
||
| testPaperImplementation("org.assertj:assertj-core:3.27.6") |
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.
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 like the AssertJ framework, we should migrate other tests to it in near future.
| Player rollczi = Objects.requireNonNull(Bukkit.getPlayer(client.getName())); | ||
| scheduler.runTask(plugin, () -> { | ||
| action.accept(rollczi); | ||
| client.quit(); | ||
| }); |
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.
The variable rollczi is used to store the player object, but the player's name is passed as a parameter name. Using a generic name like player would make the code more readable and less confusing, as this method is intended to be a generic utility for joining any player. This change should also be reflected in the test that calls this method.
| Player rollczi = Objects.requireNonNull(Bukkit.getPlayer(client.getName())); | |
| scheduler.runTask(plugin, () -> { | |
| action.accept(rollczi); | |
| client.quit(); | |
| }); | |
| Player player = Objects.requireNonNull(Bukkit.getPlayer(client.getName())); | |
| scheduler.runTask(plugin, () -> { | |
| action.accept(player); | |
| client.quit(); | |
| }); |
| joinPlayer("Rollczi", rollczi -> { | ||
| catboyService.markAsCatboy(rollczi, Cat.Type.WHITE); | ||
| assertThat(catboyService.isCatboy(rollczi.getUniqueId())).isTrue(); | ||
| assertThat(rollczi.getWalkSpeed()).isEqualTo(0.4F); |
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.
The walk speed value 0.4F is a magic number. It would be better to use a constant if one is defined in the production code (e.g., CatboyService.CATBOY_WALK_SPEED). This improves readability and maintainability. The same applies to the default walk speed 0.2F on line 31. If constants don't exist, consider adding them.
Czy to jest zarys kompatybilnosci w stylu bridge cross-version? |
Tzn? To testy E2E/integracyjne (jak zwał tak zwał)
Ale czego auto discovery?
Chodzi o pobieranie jar servera? jakiś przykład |
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.
Adjust to Gemini's suggestions
| testPaperImplementation(platform("org.junit:junit-bom:6.0.0")) | ||
| testPaperImplementation("org.spigotmc:spigot-api:${Versions.SPIGOT_API}") | ||
|
|
||
| testPaperImplementation("org.assertj:assertj-core:3.27.6") |
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 like the AssertJ framework, we should migrate other tests to it in near future.
| # https://docs.gradle.org/current/userguide/build_environment.html#sec:gradle_configuration_properties | ||
|
|
||
| org.gradle.parallel=true | ||
| org.gradle.parallel=false |
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.
@Rollczi Can't we toggle this off per-task?
|
|
||
|
|
||
| protected static void joinPlayer(String name, Consumer<Player> action) { | ||
| Litegration litegration = Litegration.getCurrent(); |
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.
The getCurrent could be changed imo on Litegration's end, "current" in english without any context means just electricity
| EternalCoreApi api = EternalCoreApiProvider.provide(); | ||
| CatboyService catboyService = api.getCatboyService(); | ||
|
|
||
| joinPlayer("Rollczi", rollczi -> { |
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.
| joinPlayer("Rollczi", rollczi -> { | |
| joinPlayer("Rollczi", player -> { |
No need to use player's name as parameter, as Rollczi is the only player anyway.
Jakubk15
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.
Approving for now, but please adjust to my and Gemini's suggestions above @Rollczi
CitralFlo
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.
Cool, check out review from Gemini
| org.gradle.caching=true | ||
| org.gradle.configuration-cache=true | ||
| org.gradle.configuration-cache.parallel=true | ||
| org.gradle.configuration-cache.parallel=false |
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.
why?
P1otrulla
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.
Looks cool!
example: