search

AuthMe / AuthMeReloaded

The best authentication plugin for the Bukkit/Spigot API!
https://www.spigotmc.org/resources/authmereloaded.6269/
GNU General Public License v3.0
632 stars 515 forks source link

SpectateLogin implementation #2745

Open Bagietas opened 1 year ago

Bagietas commented 1 year ago

Re-opened https://github.com/AuthMe/AuthMeReloaded/pull/2552 cause I don't have access to modify any of that.

ljacqu commented 1 year ago
Bagietas commented 1 year ago
  • Unit tests are missing

What do you mean by this? (Never used tests)

  • If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

Implemented it.

ljacqu commented 1 year ago

Unit tests are missing

What do you mean by this? (Never used tests)

Every class has a "Test" class associated to it, e.g. BukkitService has BukkitServiceTest. These are unit tests, which ensure that refactorings and similar don't break things in unexpected ways. Searching for JUnit should also provide additional information.

If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

Implemented it.

I think it would be important to test different scenarios, like if existing limbo player data exists before these changes, there might be limbo player data already stored on disk. Since no "gamemode" exists, it's null, which I think will cause further issues down the line when we try to set the gamemode to null on a player. I don't fully remember the whole lifecycle of the LimboPlayer (and I'm going abroad soon, so a little pressed for time this week) but I think this would be nice to really test in detail to make sure this won't break players' data. It might make sense to only touch the gamemode if the player is currently in spectator mode, the spectating setting is enabled, and we have another gamemode on the limbo player. Or maybe to have a config like we have for many of the other limbo stuff to allow it to be enabled and disabled. Someone else from the AuthMe team might have some good inputs here.

Bagietas commented 1 year ago

Unit tests are missing

What do you mean by this? (Never used tests)

Every class has a "Test" class associated to it, e.g. BukkitService has BukkitServiceTest. These are unit tests, which ensure that refactorings and similar don't break things in unexpected ways. Searching for JUnit should also provide additional information.

It's my first time I do something like tests. That's my try to implement it but I don't know if it's correct.

If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

Implemented it.

I think it would be important to test different scenarios, like if existing limbo player data exists before these changes, there might be limbo player data already stored on disk. Since no "gamemode" exists, it's null, which I think will cause further issues down the line when we try to set the gamemode to null on a player. I don't fully remember the whole lifecycle of the LimboPlayer (and I'm going abroad soon, so a little pressed for time this week) but I think this would be nice to really test in detail to make sure this won't break players' data. It might make sense to only touch the gamemode if the player is currently in spectator mode, the spectating setting is enabled, and we have another gamemode on the limbo player. Or maybe to have a config like we have for many of the other limbo stuff to allow it to be enabled and disabled. Someone else from the AuthMe team might have some good inputs here.

If I understood you correctly I've added what you've meant.