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

Fixed NoSuchMethodException when running tests on Spigot (Closes #7553) #7558

Open
wants to merge 6 commits into
base: dev/patch
Choose a base branch
from

Conversation

JakeGBLP
Copy link
Contributor

@JakeGBLP JakeGBLP commented Jan 30, 2025

Description

Added chunk-load wait before running tests (when not in DEV_MODE) that doesn't require PaperMC to Skript.class.


Target Minecraft Versions: any
Requirements: Spigot
Related Issues: #7553

@Efnilite Efnilite added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jan 30, 2025
Copy link
Member

@Pikachu920 Pikachu920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to add spigot support when we are about to drop spigot support.

@Pikachu920 Pikachu920 added enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question and removed bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Jan 30, 2025
@JakeGBLP
Copy link
Contributor Author

It's not a high effort change, for the duration of 2.10, and perhaps 2.11 if end of spigot support is delayed, I'd like this to be fixed, but even then if nothing is reverted and the end of support simply consists in not checking if the server is running paper when adding something from it then this would be very good, it would be helpful to me and anybody else looking to support spigot EVEN if skript doesn't do that itself and assuming that it loads with no issues.

@ShaneBeee
Copy link
Contributor

ShaneBeee commented Jan 30, 2025

It's not a high effort change, for the duration of 2.10, and perhaps 2.11 if end of spigot support is delayed, I'd like this to be fixed, but even then if nothing is reverted and the end of support simply consists in not checking if the server is running paper when adding something from it then this would be very good, it would be helpful to me and anybody else looking to support spigot EVEN if skript doesn't do that itself and assuming that it loads with no issues.

correct me if Im wrong, but tests are ONLY run on Paper.
After reading your changes and commenting on it.... I just wanted to point out there is no async chunk loading when running dev mode. Outside of dev mode, all tests are run on Paper servers.

@JakeGBLP
Copy link
Contributor Author

It's not a high effort change, for the duration of 2.10, and perhaps 2.11 if end of spigot support is delayed, I'd like this to be fixed, but even then if nothing is reverted and the end of support simply consists in not checking if the server is running paper when adding something from it then this would be very good, it would be helpful to me and anybody else looking to support spigot EVEN if skript doesn't do that itself and assuming that it loads with no issues.

correct me if Im wrong, but tests are ONLY run on Paper. After reading your changes and commenting on it.... I just wanted to point out there is no async chunk loading when running dev mode. Outside of dev mode, all tests are run on Paper servers.

The goal of this PR is to handle the scenario in which an addon were to run tests on Spigot since Skript only runs them on Paper.

@TheAbsolutionism
Copy link
Contributor

TheAbsolutionism commented Jan 30, 2025

I don't think it makes sense to add spigot support when we are about to drop spigot support.

In my opinion, Jakes reasons are valid. Even though Skript does plan on dropping support for Spigot, nothing has been said as what version of Skript will be doing so. If the team does plan on dropping support for 2.11 or any version after, regardless, 2.10 still supports Spigot. So we should be fixing any bugs that occur before parting ways. Meaning we need to fix the test environment (this PR) in order to find and fix any bugs/errors.
For instance, both Shane and Sovde had tested on Spigot and found bugs which PRs were made to fix and have been merged for 2.10.

@TheAbsolutionism
Copy link
Contributor

With that said, the labels need to be changed. Removing enhancement and adding back bug so it can appropriately be marked and merged for the next patch release.

@ShaneBeee
Copy link
Contributor

With that said, the labels need to be changed. Removing enhancement and adding back bug so it can appropriately be marked and merged for the next patch release.

Its not a bug tho.
Jake copy/pasted Skript's Test platform runner classes, and added his own environments... this isn't something Skript was meant to do.
There is no bug here, Skript's testing environment was setup to use Paper, and works as it was intended to.
Nothing is broken.

Requested from @sovdeeth

Co-authored-by: sovdee <[email protected]>
@JakeGBLP
Copy link
Contributor Author

JakeGBLP commented Jan 31, 2025

With that said, the labels need to be changed. Removing enhancement and adding back bug so it can appropriately be marked and merged for the next patch release.

Its not a bug tho. Jake copy/pasted Skript's Test platform runner classes, and added his own environments... this isn't something Skript was meant to do. There is no bug here, Skript's testing environment was setup to use Paper, and works as it was intended to. Nothing is broken.

I think it's more of something nobody wanted to be bothered to do, spigot support has almost become an optional over the last few updates, that being said I think having spigot tests is ideal for a Spigot-supporting plugin, so if that were to be the plan it would be convenient to have them; some people don't love having many testing environments, I don't mind it at all, it's very helpful since it's fully automated (once you set it up) and makes tests all the more worthwhile since we go from running a few environments to a bunch of them if not ALL of them.

TL;DR: if we consider Skript as a spigot plugin then this is a bug.

@JakeGBLP JakeGBLP requested a review from sovdeeth February 2, 2025 10:50
@sovdeeth
Copy link
Member

sovdeeth commented Feb 2, 2025

this isn't a bug, period. That said, using paperlib to have broader support without, like, any downside seems like a fine enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants