Z order violation

[Jaggeroth]

This solves the ZOrder sort violation problem by restoring the original comparator and only using the new figure comparator when sorting figure only.

Difficult to say this absolutely solves the problem as we have never been able to reliably trigger the error.

---

This change is 

[irisiflimsi]

Looks okay. BTW, the test that I posted reliably triggers the problem:-). The section o1.isStamp && o2.isToken relies on the fact that factually isToken = !isStamp. If this were not the case in the future, we will have the problem again. I advise to change either isToken or isStamp to the reverse of the other.

---

Comments from Reviewable

[Azhrei]

I think I agree. On the one hand, it might read better to use isStamp() and isToken(), but if the list of token types should change in the future, it’s likely that a type other than Stamp and Token would fall through the cracks. I’d say pick one and replace the other with “! ...”

And yes, the code iris put in the issue repeatedly recreates the problem, although I have to admit the use of random means it’s not guaranteed (although the larger the sample set, the more likely it is).

Which just goes to show we need more unit testing! The MT code that exists now makes it very difficult to create mockups necessary for such testing, but future architectural changes (i.e. changes to the class diagram) will require unit tests before they’re merged. Plan for dependency injection now. ;)

[irisiflimsi]

The reason I chose random is that to reliably get timsort to break with a broken comparator is not that easy. In fact, 100 random tokens didn't work, I think. It also has the benefit that you can easily add other pertinent calls to token methods from the comparator to test it, without having to understand a root cause. I mean, if you knew a potential root cause, you can write a test for it, but the other way?

[Azhrei]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Azhrei]

I understand that 100 wasn't enough and that 1000 were needed. My point was that using random data isn't a good way to test code because you can't guarantee that it will generate the conditions needed to trigger a bug.

However, a true mockup would be a PITA because you'd have to hand-code a bunch of specific values to test all parts of the code and for something like this comparator, even a code coverage tool isn't going to help much. (It'll tell you whether you took all branches of an IF statement or SWITCH statement, but that isn't helpful in trigger a contract failure like this one.)

Anyway, it's good to have the test code and I may work it into a unit test at some point. I should probably search for all other comparators and tests for them as well... This is why procrastinators are never bored -- there's always something to do. ;)