UNIX Compatibility 2: Electric Boogaloo

[3d12]

I definitely did not fully test my last commit,
so this commit resolves the issue with the
extractAllFiles() method on UNIX-based
filesystems.

[Frotty]

Can you please add a unit test then?

[coveralls]

Coverage remained the same at 84.857% when pulling 8ab0eac on 3d12:master into 475e84e on inwc3:master.

[3d12]

Not to be that guy, but do you have a reference I could use in developing that unit test then? I'm still very, very new to Java... I think that would be using JQuery, right?

[3d12]

Sorry, I realize that comment may have seemed dismissive at a second glance. I would definitely be interested in learning how to write a unit test. The stuff in the src/test folder doesn't seem to difficult, but I'm not sure how it's supposed to actually check anything? Does gradle take care of running all of those?

Honestly, this is going to be really embarrassing to admit, but I couldn't even figure out how to turn logging on, so I had to debug this one by hand -- that's why it took me 4 days to change 2 lines of code... 😡 🤕

[Frotty]

Java has nothing to do with JavaScript, and therefore also jQuery. The testing framework we use is https://testng.org/doc/index.html but you don't need to read its docs to make a test 😄

You can copy of of the tests from MPQTests.java. I suspect you committed this fix because you had issues using jmpq? So make a test where you call the api like you did in your case, and try to verify if it errors without your fixes. Use Assert.assertXXX to assert that e.g. the mpq got opened correctly or your file extracted as expected. Then validate that you fixes also fix the test case.
If so, push to pr branch so travis can check as well.

Thanks for your efforts 👍

[3d12]

Ah, I see. Looking at my Java notes, it was JUnit I was thinking of, not JQuery. Still, thanks for pointing that out! 😄

With all respect, I'm not sure this issue is something that can be unit-tested for. Or maybe it already has?

Here's a link to the output of the out/ directory after running gradle test on the current master branch: https://i.imgur.com/eMO4nWt.png

I believe this is the result of the testExtractAll() test, since I didn't add a test yet. But I think this conclusively proves my issue?

Here's the output of gradle test on my master branch (in this PR): https://i.imgur.com/eytvNH6.png

Again, no tests added by me so far. 😃

How do I push to pr branch? I don't think I have direct push, so do I just open a new PR targeting that branch instead? Disregard, I just realized what you meant -- push to this branch so the CI system can check any changes to tests.... Too early to be learning this many new things... ☕️ 😴

[Frotty]

If there already would be a test, it should have failed with the unix incompatibilities.
If it doesn't happen on travis CI, which is linux, there might be another problem at hand.

[3d12]

Yeah, that's why I figured we couldn't unit test for this. Per my screenshot, a file gets created in either case with the contents of the listfile as the name. If we test for the file existing, it will exist.

We would have to test the filename of the created file and ensure it matches only the very last part of the path if we wanted to validate. Basically, we need a way to tell that out/'war3mapImported\Double_Kill.mp3' and out/war3mapImported/Double_Kill.mp3 are two different files.

As written, any tests that validate the existence of the file by matching on the name will surely pass unless a distinction is made between the filename and the relative path from the contents of the listfile.

[Frotty]

Hey sorry, I totally forgot about this, I think I'm not getting emails.
Is this still current? Did you do tests?

@3d12

[3d12]

Hi @Frotty

No, I haven't added tests. I haven't done unit testing in Java yet so I might be off-base here, but I don't believe there's a way to test for this condition, based on the images I included in my last post.

Any tests that validate the existence of the file by matching on the name will pass unless a distinction is made between the filename and the relative path from the contents of the listfile. And I'm not sure how to make such a distinction in a test. Sorry.

But you can probably tell from the images how this bug works, so if you'd like to take a shot at writing the test I would be eager to see how you would approach this.

[Frotty]

I guess this is still useful, sorry for not responding.