Feature Add unit test for the skill parser in CharSheetRequest.java

[philmasterplus]

This patch adds a new unit test for CharSheetRequest.java.

• Added a new class, CharSheetRequest.ParsedSkillInfo. It's a simple POJO class representing a parsed (but not identified) skill.
• Extracted the skill parsing code to a method, CharSheetRequest.parseSkills(). This method takes an XML Document node as argument and returns an ArrayList<ParsedSkillInfo>.
• Added a unit test that exercises this method. The accompanying test_charsheet.html has been scrubbed of any real-world user names and actual pwd hashes (replaced with dummy hashes).

I also modified build.xml so that unit tests could import JARs in src/jar/. I did this because the unit test needed to use HTMLCleaner. I hope I'm not breaking anything by modifying build.xml...

Tested on r20582.

P.S. While testing this, DataFileTest.java kept failing for me, so I had to disable it (patch not included). I assume its unrelated to my patch.

 

[fronobulax]

DataFileTest fails for me without your patch so we should probably figure out why.

I thought tests already had access to the same jars that the "sources" did, but if that was not your experience I am probably wrong. Nevertheless @MCroft is the keeper of the Build file so...

I may look at the test ASAP because identifying how/why it happened is an opportunity to improve process but if anyone else wants to look at this that's fine with me.

 

[fronobulax]

Jowls Full, and Belly seems to be the first status effect with a comma. In any event the test and associated regex seem to be what is wrong, not anything else.

 

[MCroft]

DataFileTest fails for me without your patch so we should probably figure out why.

I thought tests already had access to the same jars that the "sources" did, but if that was not your experience I am probably wrong. Nevertheless @MCroft is the keeper of the Build file so...

It's the same requirement as the code: they jars are available at runtime, but this new test is requiring the src/jar directory at build time, so this change is both correct and required for the new test to use +import net.sourceforge.kolmafia.utilities.HTMLParserUtils;. I had already made it for my requirements in my sandbox, but that code is currently in shambles...

I haven't reviewed anything else, but I have been going slightly mad trying to figure out why the DataFileTest.java was failing in my JUnit 5 conversion, and now I think it's because I didn't check that it was working in JUnit 4...

 

[Ryo_Sangnoir]

Here's a fix to DataFileTest.java. Some of the entries in the data files had spaces instead of tabs: I assume somebody's editor was playing silly buggers.

I've also adjusted one of the error messages to be slightly more helpful: fields[1] is right for items but useless for the others.

 

[fronobulax]

Here's a fix to DataFileTest.java. Some of the entries in the data files had spaces instead of tabs: I assume somebody's editor was playing silly buggers.

I've also adjusted one of the error messages to be slightly more helpful: fields[1] is right for items but useless for the others.

Thanks. On my list for later today.

I have a couple tools that will display white space so I can see the difference between a space and a tab but I didn't notice any examples in a quick scan.

Seems to me that mafia has some built in check* commands that will find space vs tab type of things. Maybe in that future time when we run tests as part of a commit, we extend that to run check commands if certain datafiles change.

 

[MCroft]

.

Seems to me that mafia has some built in check* commands that will find space vs tab type of things. Maybe in that future time when we run tests as part of a commit, we extend that to run check commands if certain datafiles change.

I was thinking about making a JUnit test that called these, but Veracity said they were tricky and required manual review if they failed.

 

[fronobulax]

I was thinking about making a JUnit test that called these, but Veracity said they were tricky and required manual review if they failed.

Agreed but I can imagine a modified command that is less comprehensive but suitable for automation.

I can also imagine new commands/tests that just focus on format. If a data file has lines that don't have at least 3 fields and less than 6 delimited by tabs then the test fails, for example.

 

[Veracity]

What I meant to say is that there are known failures. For example, for modifiers that only apply in a particular zone. Or effects that give random stat adjustments.

Presumably the solution would be to code the DebugDatabase functions to Do the RIght Thing for those. Somehow.

 

[fronobulax]

Here's a fix to DataFileTest.java. Some of the entries in the data files had spaces instead of tabs: I assume somebody's editor was playing silly buggers.

I've also adjusted one of the error messages to be slightly more helpful: fields[1] is right for items but useless for the others.

r20584

As a philosophy, I think it is better to report a failing test than to comment it out. If there is a future where commits and/or build success is linked to passing tests, commenting out a failing test and committing is almost always the Wrong Thing.

 

[MCroft]

What I meant to say is that there are known failures. For example, for modifiers that only apply in a particular zone. Or effects that give random stat adjustments.

Presumably the solution would be to code the DebugDatabase functions to Do the RIght Thing for those. Somehow.

I am in favor of this. This would also be a pretty good place to generate some JUnit tests. I didn't know about DebugDatabase, and yeah, it looks like it gives us a lot of value in terms of validating our data structures.

r20584

As a philosophy, I think it is better to report a failing test than to comment it out. If there is a future where commits and/or build success is linked to passing tests, commenting out a failing test and committing is almost always the Wrong Thing.

I guess we need to dig into the one in MaximizerTest that got commented out in 2019. It probably causes a lot of our "why did it recommend 'X'?" confusion.

I think we can at least use the @ignore("reason") annotation (as was added 10 days ago).

that said, every other tests passes, in r20584

test:
    [junit] Running net.sourceforge.kolmafia.CustomScriptTest
    [junit] Testsuite: net.sourceforge.kolmafia.CustomScriptTest
    [junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.503 sec
    [junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.503 sec
    [junit]
    [junit] Running net.sourceforge.kolmafia.DataFileTest
    [junit] Testsuite: net.sourceforge.kolmafia.DataFileTest
    [junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.711 sec
    [junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.711 sec
    [junit]
    [junit] Running net.sourceforge.kolmafia.MaximizerTest
    [junit] Testsuite: net.sourceforge.kolmafia.MaximizerTest
    [junit] Tests run: 7, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 5.909 sec
    [junit] Tests run: 7, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 5.909 sec
    [junit]
    [junit] Testcase: noTieCanLeaveSlotsEmpty(net.sourceforge.kolmafia.MaximizerTest):SKIPPED: Currently failing
    [junit] Running net.sourceforge.kolmafia.utilities.BooleanArrayTest
    [junit] Testsuite: net.sourceforge.kolmafia.utilities.BooleanArrayTest
    [junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec
    [junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.01 sec
    [junit]

 

[philmasterplus]

I appreciate all your hard work, but...are you going to merge my patch?

If there's a problem with it, I could try to fix it.

 

[MCroft]

I appreciate all your hard work, but...are you going to merge my patch?

If there's a problem with it, I could try to fix it.

I thought Frono did already, but that was @Ryo_Sangnoir 's patch. I'll take a look at yours.

 

[fronobulax]

I appreciate all your hard work, but...are you going to merge my patch?

If there's a problem with it, I could try to fix it.

No, or rather not yet. The patch conflicted with Ryo's on my system and then Christmas came. Now I will gladly watch @MCroft ort it out.

 

[MCroft]

No, or rather not yet. The patch conflicted with Ryo's on my system and then Christmas came. Now I will gladly watch @MCroft ort it out.

You’re top of the list, ahead of “fold the fitted sheets”...

Merged. It's a good start, but it has 1 test in it and could reasonably test more things, and might be better off with a reduced test case. You want a failure of the test to be very clear about what you expected and what actually happened.

 

[philmasterplus]

Thank you.

I went back and forth on whether it is appropriate to pass an entire HTML file as the test input. I eventually decided yes, since the skill parsing code is meant to process charsheet.php. Passing a small crafted HTML fragment as the test input would not exercise the code in the right way.

I did implement ParsedSkillInfo.equals() and ParsedSkillInfo.toString() so that assertArrayEquals() would give meaningful diff messages. I hope that would be enough.

 

[MCroft]

I'm in favor of how you're getting the data as a functional test. Others may want to add unit tests for the functions you list.

What I'm advocating for, if anything, is smaller tests that are easier to tell what's wrong, as well as any other parsing issues.

These are just suggestions. I'm glad to have any tests, and I don't want to look a gift horse in the mouth, but I didn't want to change it up without talking to you, in case you were considering additional changes.

Here's what I want to do differently.
1: test fewer skills -- one each of non-permed, software permed, hardcore permed
2: test skills that broke in the past -- like the ones with duplicate names.
3: individual asserts for each skill
4: asserts should have messages for failures (e.g. "Entangling Noodles should be Permed, but reports HP", etc.).
5: assertFalse for negative tests -- e.g. new skill (to cover isBadId(), etc.), non-matching, duplicate. Might require another char sheet.
5: add a test for parseStatus.
6: add and parse a char sheet for a brand new character (to check when they have almost no skills, no meat (if that's even parsed)

 

[fronobulax]

In the past and in other environments, there was a type of test that I found less than useful.

The test used a static data file. Often the test would have regular expressions embedded in the test.

The utility was limited for a couple of reasons. The first is that the test could never detect that the real world equivalent of the data file had changed. The second was that the test often contained code that was the equivalent of code in the application but copied or duplicated because exposing it to the test was difficult. So there were failing tests that were fixed by changing the test code, not the application code.

 

[MCroft]

In the past and in other environments, there was a type of test that I found less than useful.

The test used a static data file. Often the test would have regular expressions embedded in the test.

The utility was limited for a couple of reasons. The first is that the test could never detect that the real world equivalent of the data file had changed. The second was that the test often contained code that was the equivalent of code in the application but copied or duplicated because exposing it to the test was difficult. So there were failing tests that were fixed by changing the test code, not the application code.

Sure, and it's a good point. It gets hard when you have to actually use a data connection/account instead of a mock/static file, though. You're looking at a different scope of tests (functional vs. integration). When we had these kinds of issues at places I used to work we had non-production environments with our external partners to provide exactly this integration testing coverage and yes, we often skimped on integration testing once we had a production environment, but prior to launch, when the partner's code didn't exist, testing with data files was all we had.

So there are four classes of tests:

1. unit tests which test a function (e.g. isBadId() )
2. functional tests which do not require a user login (e.g. Maximizer style tests).
3. Integration tests which require a connection to an external system but are stateless (check commands which hit the wiki...).
4. Integration tests which require a connection to an external system with state (a login, like KoL).

Do we have a test environment/account we can log into/pull a char sheet from to run the class of tests that require you to be logged in? If not, static data will catch some things,