simon987 / Much-Assembly-Required

Assembly programming game
https://muchassemblyrequired.com
GNU General Public License v3.0
925 stars 87 forks source link

The file 'config.properties' cannot be found inside tests #200

Closed kevinramharak closed 5 years ago

kevinramharak commented 5 years ago

So after a deep dive into the rabbit hole I found out why my tests are mysteriously failing. This is because of surefire using forks to run tests, where the IOException gets caught, logged and forgotten.

Adding <forkCount>0</forkCount> to the Server/pom.xml plugin configuration of surefire got me a step further. Adding try catches and a forced assert failure inside the catch to the 6 tests that use this file got me the following output:

  MemoryTest.getSet:17 config.properties (The system cannot find the file specified)
  MemoryTest.write:42 config.properties (The system cannot find the file specified)
  AddInstructionTest.addTargetImm:142 config.properties (The system cannot find the file specified)
  AddInstructionTest.addTargetTarget:25 config.properties (The system cannot find the file specified)
  AndInstructionTest.executeTargetImm:68 config.properties (The system cannot find the file specified)
  AndInstructionTest.executeTargetTarget:21 config.properties (The system cannot find the file specified)

@simon987 Do you have any idea what change could have made my windows system fail to find that file. As the file is perfectly fine and in the correct place, I have no idea why the file cannot be found.

EDIT: surefire generates the following file with the working directory set correctly:

#surefire_0
#Tue Apr 23 19:18:39 CEST 2019
basedir=C\:\\Users\\kevin\\Projects\\Much-Assembly-Required\\Server
user.dir=C\:\\Users\\kevin\\Projects\\Much-Assembly-Required\\Server\\src\\main\\resources
localRepository=C\:\\Users\\kevin\\.m2\\repository
kevinramharak commented 5 years ago

ok found the problem, for some reason this line in https://github.com/simon987/Much-Assembly-Required/blob/master/Server/src/main/java/net/simon987/server/ServerConfiguration.java#L27 will not use the working directory (the user.dir property above) when looking up config.properties.

By manually prepending the file path like InputStream is = new FileInputStream(System.getProperty("user.dir") + "\\" + this.fileName); the code will run as expected. @simon987 Any ideas on how to fix this without breaking other peoples builds?

simon987 commented 5 years ago

Ideally, the ServerConfiguration file would try to find the file in the working directory, then if it doesn't work, it would use user.dir (Like you did, but with a platform-independent path separator). Is this something you can do in a PR @kevinramharak ? I don't have a dev environment nearby

kevinramharak commented 5 years ago

The thing is that the pom.xml correctly defines the working directory with:

                    <workingDirectory>./src/main/resources</workingDirectory>

And when dumping the System.getProperty('user.dir') to output it works fine and even the ugly workaround above works.

Ill try to get to work out the nice options before resolving to the fix you propose. And no problem ill make a PR, since it seems to only plague my system anyway.

kevinramharak commented 5 years ago

Well problem found. Even tough the working directory is set and System.getProperty("user.dir") returns the correct value, the new File() (and probably new FileInputStream()) API seem to use the working directory of where the Java VM was started. Apparently this is working as intended in Java 11 and guess what version I am using :D

@simon987 The options to fix it:

This seems to only be a problem when testing because of Surefire explicitly setting the working directory (and thus failing in Java 11). I am not sure how production code handles this case.

kevinramharak commented 5 years ago

Well, new problems arise. The ServerConfiguration("config.properties") line is found in multiple files. Also when attaching my debugger in VS Code the generated user.dir by the pom.xml does not get set so it is kinda hard to debug.

I assume it works in production because the config.properties file is placed next to the server.jar file. Which then is the working directory thus the file resolves without a problem. This makes it only a problem for testing in > Java 11 environments.

The files using the code are:

Other classes retrieve the singleton GameServer.INSTANCE to retrieve its configuration and as long as that pattern stays it will work.

I think the best way to solve this is to edit the tests to build a path from the project root and pass that to the new ServerConfiguration() constructor, as for now the tests are the only problem.