Bug ConcoctionTest.itShouldSortUsables sometimes failing

[heeheehee]

So, I'm not sure exactly how to reproduce this in a platform-independent way, but I do sometimes see failures for this one test.

Typically of the form:

org.opentest4j.AssertionFailedError: expected: <10869> but was: <0>
    at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
    at app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
    at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
    at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
    at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:527)
    at app//net.sourceforge.kolmafia.objectpool.ConcoctionTest.itShouldSortUsables(ConcoctionTest.java:30)
...

One thing I've noted is that `thing = usableList.getSize()` (all visible elements) is being compared against `usableList.size()` (all elements). @fronobulax what is the intent of this test? It's not clear to me what it should be (although from the naming it probably shouldn't be concerned with element visibility).

If I change .getSize() to .size() it passes, although it still doesn't assert what I'd expect it to (namely that the resulting list is in sorted order).

(I also have no idea how this list's elements are sometimes just not visible, although that would help with understanding how to reproduce the test failures.)

 

[fronobulax]

So, I'm not sure exactly how to reproduce this in a platform-independent way, but I do sometimes see failures for this one test.

Typically of the form:

org.opentest4j.AssertionFailedError: expected: <10869> but was: <0>
    at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
    at app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
    at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
    at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
    at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:527)
    at app//net.sourceforge.kolmafia.objectpool.ConcoctionTest.itShouldSortUsables(ConcoctionTest.java:30)
...

One thing I've noted is that `thing = usableList.getSize()` (all visible elements) is being compared against `usableList.size()` (all elements). @fronobulax what is the intent of this test? It's not clear to me what it should be (although from the naming it probably shouldn't be concerned with element visibility).

If I change .getSize() to .size() it passes, although it still doesn't assert what I'd expect it to (namely that the resulting list is in sorted order).

(I also have no idea how this list's elements are sometimes just not visible, although that would help with understanding how to reproduce the test failures.)

I suspect it was designed more for coverage and reintroducing me to testing. I am almost certain I never considered the possibility that the list and sorted list would have different lengths. I will look at this soon although, as you noted, it is not a failure that happens on all platforms on, all of the time

 

[heeheehee]

The list and sorted list have the same size, but for some reason, the LockableListModel sometimes has no visible elements. I don't know what's causing that.

 

[fronobulax]

/* This test was triggered by a runtime error traced back to sorting usable concoctions that
said "Comparison method violates its general contract!"

 

[Veracity]

I hate that. I have been known to actually quote the text of what the "contract" is in "compare" methods.
I am going to be really embarrassed if one such method is at fault.

Reminds me of a bug I tracked down at work, which was on a line that had a comment that said "this is the right way to do this".
It was not the right way to do this.

 

[fronobulax]

I hate that. I have been known to actually quote the text of what the "contract" is in "compare" methods.
I am going to be really embarrassed if one such method is at fault.

Reminds me of a bug I tracked down at work, which was on a line that had a comment that said "this is the right way to do this".
It was not the right way to do this.

The test either follows your example or copied your quotes. The original inspiration for the test was an exhaustive verification that the contract was being met under the assumption that the data was driving the violation and not the code.

My belief now is that the contract validation is appropriate but it is probably better done by an optional command rather than a test that gets run several times a day by developers who actually choose to test all code before they push their code

That vision hasn't happened yet because a brute force iteration with three nested loops with over 10,000 items each takes up resources my computer cannot provide. But I should be able to address that "soon".

 

[heeheehee]

The only contract violation I could find from an earlier skim was compareTo not being compatible with equals (namely a.compareTo(b) == 0 but !a.equals(b)), which should only impact insertion into a Set or as the key of a Map.

There were no cases that I found where Concoctions were used in that context.

 

[fronobulax]

I got several contract violations where two different concoctions make the same named item - Ed staff components are several examples. I am working with a fix that uses the itemID to make the compare if the names match. https://github.com/kolmafia/kolmafia/pull/207 for the curious. I am getting an index out of bounds in the setup portion that I have not tracked down yet but seems to be an issue with the length of the LockableListModel. Working on that.