bernardosulzbach / dungeon

Text-based open-world RPG made with Java
https://www.bernardosulzbach.com/dungeon
BSD 3-Clause "New" or "Revised" License
144 stars 52 forks source link

Shouldn't require a path with no spaces for the tests to pass #419

Closed cPolidore closed 4 years ago

cPolidore commented 4 years ago

An attempt to build the game locally from the master branch resulted error due to a parsing issue with org.codehaus.moho:findbugs-maven-plugin:3.0.3

This can be fixed by changing the version of the plugin to 3.0.4 as is reported here:

https://stackoverflow.com/questions/53676071/maven-clean-command-java-util-collections-unmodifiablerandomaccesslist-to-prope/55834783

After change, attempt to build resulted in test failures during ...io.ResourcesFolderTest. Download link for log from test included below:

org.mafagafogigante.dungeon.io.ResourcesFolderTest.txt

bernardosulzbach commented 4 years ago

OK. What version of Java are you using? How are you building the project?

I do plan to "bump" the Java requirement for the project so that it can move on, but I am not sure to what version yet.

cPolidore commented 4 years ago

I am using JDK8 (version 1.8.0_171). I'll find a download and install JDK7 over the weekend to see if that fixes the issue unless you don't think it will make a difference. My Maven version is 3.6.2. I am running on a Windows system which could be causing an issue as well. If changing to JDK7 doesn't correct the issue, I can spin up a Linux box without too much difficulty.

To build, I downloaded the project (for the most recent attempt, I think I downloaded the .zip and unpacked it, but I'd previously used git clone and ran into similar issues) from the master branch on the main page to a test folder I made for the purpose, changed my directory to dungeon, and ran "mvn package".

bernardosulzbach commented 4 years ago

OK, thank you for your reply.

I'll find a download and install JDK7 over the weekend to see if that fixes the issue unless you don't think it will make a difference.

I don't think so, no. In fact, Travis CI builds the project using JDK 7 and 8 and both seem to work OK.

I don't build on Windows, neither does the CI pipeline. However, this shouldn't matter.

These are the Java versions I am using to build this locally. It works as intended. Right now, I think Java 11 (and probably everything after that) does not work. This is why I asked about which version you were using.

bernardo@workstation:~/code/dungeon> java -version
openjdk version "1.8.0_222"
OpenJDK Runtime Environment (IcedTea 3.13.0) (build 1.8.0_222-b10 suse-2.1-x86_64)
OpenJDK 64-Bit Server VM (build 25.222-b10, mixed mode)

bernardo@workstation:~/code/dungeon> javac -version
javac 1.8.0_222

I have never seen the issue your build logs show. Let's figure out what is causing this.

bernardosulzbach commented 4 years ago

I am changing findbugs to spotbugs (seems to be the new name for the tool).

<groupId>org.codehaus.mojo</groupId>
<artifactId>findbugs-maven-plugin</artifactId>

will become

<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs-maven-plugin</artifactId>

and this should land on master very soon.

bernardosulzbach commented 4 years ago

Forgot Maven versions...

Locally:

bernardo@workstation:~/code/dungeon> mvn --version
Apache Maven 3.5.4 (SUSE 3.5.4-1.5)

Travis CI:

Apache Maven 3.6.0 (97c98ec64a1fdfee7767ce5ffb20918da4f719f3; 2018-10-24T18:41:47Z)

I don't think Maven can be the cause of your problems.

cPolidore commented 4 years ago

I found out what was causing the test failures: there is a space character in my file path (which I've been meaning to change for ages). ResourcesFolderTest was constructing the path name with "%20". The folder was returning false for isDirectory() because as far as it could tell, the folder didn't exist. Hard coding a String with a path including the space character resulted in no errors. If I change my upper level folder structure to omit space characters, it should correct the problem.

ResourcesFolderTest could do a check for existence to return a more accurate message in case this happens to someone else, but that is probably unnecessary as my first assumption was that it wasn't pathing correctly.

@bernardosulzbach: added emphasis on the cause of the issue to be easier to find.

bernardosulzbach commented 4 years ago

Ah! I just reproduced it.

bernardo@workstation:~/code> cp -r dungeon "dungeon with spaces"
bernardo@workstation:~/code> cd "dungeon with spaces"
bernardo@workstation:~/code/dungeon with spaces> mvn test

This is indeed a bug, thank you for helping me discover it!

bernardosulzbach commented 4 years ago

Right now it reports "Assertion Error: Resources file should be a directory", which is useless.

Immutablevoid commented 4 years ago

Using URLDecoder seems to fix this problem:

resourcesDir = new File(URLDecoder.decode(resourcesUrl.getFile(), "UTF-8"));

Ill create a pull request with my full findings.