-
Notifications
You must be signed in to change notification settings - Fork 423
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
Add various lifetime-bound try-with-resources APIs to client gametests #4318
Add various lifetime-bound try-with-resources APIs to client gametests #4318
Conversation
context.takeScreenshot("server_config"); | ||
server.runCommand("debugconfig unconfig " + profile.getId()); | ||
// TODO: better way to wait for reconfiguration to end | ||
context.waitTicks(100); |
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.
Not having this waitTicks(100)
here breaks the assertion in TestServerConnection.close()
because reconfiguration sets MinecraftClient.world
to null
. Ideally I want the close()
method to wait for any configuration to finish before trying to disconnect. I tried to fix this for this PR, but I didn't manage to figure out a good way to fix it, so I've left the problem for later. I imagine it's a pretty rare case anyway.
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.
Yeah, could you maybe wait until the world has rendered? I dont think it matters a vast amount, as this test is something that I dont expect many if anyone to run. I think I added it after we broke reconfiguration, im not even sure the debugconfig command is registered by default.
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.
This is really great, my only slight worry is around agreeing to eula for people.
* | ||
* @param command The command to run | ||
*/ | ||
void runCommand(String command); |
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.
A thing for the future, but getting the result of the command might be handy, not quite how it would work but I could see it being useful.
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.
This function was just copied over from TestDedicatedServer
. But yes, could be handy. Annoyingly the current function it's implemented with doesn't return the result of the command but it should hopefully be a case of finding a different function to use.
...api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/DedicatedServerImplUtil.java
Outdated
Show resolved
Hide resolved
context.takeScreenshot("server_config"); | ||
server.runCommand("debugconfig unconfig " + profile.getId()); | ||
// TODO: better way to wait for reconfiguration to end | ||
context.waitTicks(100); |
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.
Yeah, could you maybe wait until the world has rendered? I dont think it matters a vast amount, as this test is something that I dont expect many if anyone to run. I think I added it after we broke reconfiguration, im not even sure the debugconfig command is registered by default.
See changes to
ClientGameTestTest
for an overview of the changes to the API.