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

Allow fabric-loader-junit EnvType to be changed #806

Merged
merged 4 commits into from
May 15, 2023

Conversation

LCLPYT
Copy link
Contributor

@LCLPYT LCLPYT commented May 9, 2023

This allows the EnvType in FabricLoaderLauncherSessionListener to be tweaked via the fabric.side system property.
With this, server-only mods can also be unit tested.
Closes #804.

Minor detail: It would be better to introduce a utility method somewhere that parses the fabric.side property.
I essentially use the same switch block as in Knot#init. However, introducing the method to the fabric-loader subproject would also require a new version of fabric-loader for such a minor feature. Therefore, I've duplicated the parser for now.

Usage

One way to tweak the unit-test EnvType, in Gradle buildscripts:

tasks {
    test {
        systemProperty("fabric.side", "server")
    }
}

I will also open a pull request on loom for automatically adding the property when loom.serverOnlyMinecraftJar() is configured.

@modmuss50
Copy link
Member

I handled this in my loader PR here: #776
And loom PR here: FabricMC/fabric-loom#835

I dont believe setting the system property like that in Gradle applies when the tests are run via the IDE, especially when running indiviual tests. Thus why a solution like mine above is needed. I still need to finish off and test my two PRs before merging them.

@LCLPYT
Copy link
Contributor Author

LCLPYT commented May 9, 2023

I see, that makes sense.

For IntelliJ IDEA at least, I can confirm that the IDE executes the buildscript each time before the tests are executed (also for single tests, because they are invoked with :test --tests "foo.bar.Test.testMethod)

image

I don't know about VSCode and Eclipse thought... So your approach is probably more compatible.

@modmuss50
Copy link
Member

Oh nice, I wasnt aware of that! I imagine if you change this setting it wont work:

Screenshot 2023-05-09 at 20 40 48

However I much prefer this approach over my complex solution 🤔 Ill have a closer look at this shortly. Thanks

VSCode and Eclipse dont support these tests.

@LCLPYT
Copy link
Contributor Author

LCLPYT commented May 9, 2023

Yeah, if you select "Run tests using IntelliJ IDEA", it does not use the buildscript.

Maybe both solutions could be used. Loom could configure the test task to have fabric.side=server if loom.serverOnlyMinecraftJar() was specified. That would work, as long as tests are running using Gradle.

If running through the IDE, I think your solution is defenitely needed (unless extra system properties for tests can somehow be defined in the .iml or something...)
Maybe loom could generate the config file automatically in that case? I haven't looked into your loom PR, sorry if that is already the case 😄

But if there would be an API for the loom extension, I if think that would also be pretty nice to use.
Something like:

loom {
  test {
    systemProperty("fabric.side", "server")  // auto configure according to serverOnlyMinecraftJar()
    systemProperty("foo", "bar")
  }
}

In that case my PR would probably be obsolete...

@modmuss50
Copy link
Member

I dont think your PR is obsolete in anyway, In my PR I failed to notice the existing system prop for setting the env. And the loom changes required for this PR should be minimal and much lower risk. This PR also does not prevent a change like mine from being done later down the line.

LCLPYT and others added 2 commits May 10, 2023 02:47
…rLauncherSessionListener.java


use default value instead of Optional API

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

LCLPYT commented May 10, 2023

You are right, this PR is not obsolete. What I meant was the configuration of the test task by loom, which might be redundant if there already is a config file containing the property. Anyways, I think this PR is ready. I did some manual testing and it seems to work without the optional API just fine.

…rLauncherSessionListener.java


Remove unused parseEnvType method

Co-authored-by: modmuss50 <[email protected]>
@LCLPYT LCLPYT requested a review from modmuss50 May 11, 2023 19:24
@modmuss50 modmuss50 merged commit 3d26d17 into FabricMC:master May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JUnit testing with server-only mods fails to locate the game
3 participants