Unclear expected values for integer division

Mingun commented 8 months ago

Currently Java test tests\formats\expr_int_div.ksy is failed with the following message:

java.lang.AssertionError: expected [-757] but found [-756]
    at org.testng.Assert.fail(Assert.java:94)
    at org.testng.Assert.failNotEquals(Assert.java:513)
    at org.testng.Assert.assertEqualsImpl(Assert.java:135)
    at org.testng.Assert.assertEquals(Assert.java:116)
    at org.testng.Assert.assertEquals(Assert.java:389)
    at org.testng.Assert.assertEquals(Assert.java:399)
    at io.kaitai.struct.spec.CommonSpec.assertIntEquals(CommonSpec.java:36)
    at io.kaitai.struct.spec.TestExprIntDiv.testExprIntDiv(TestExprIntDiv.java:17)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:607)
    at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:86)
    at org.testng.internal.Invoker.invokeMethod(Invoker.java:643)
    at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:820)
    at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1128)
    at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
    at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112)
    at org.testng.TestRunner.privateRun(TestRunner.java:782)
    at org.testng.TestRunner.run(TestRunner.java:632)
    at org.testng.SuiteRunner.runTest(SuiteRunner.java:366)
    at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:361)
    at org.testng.SuiteRunner.privateRun(SuiteRunner.java:319)
    at org.testng.SuiteRunner.run(SuiteRunner.java:268)
    at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
    at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
    at org.testng.TestNG.runSuitesSequentially(TestNG.java:1244)
    at org.testng.TestNG.runSuitesLocally(TestNG.java:1169)
    at org.testng.TestNG.run(TestNG.java:1064)
    at org.testng.TestNG.privateMain(TestNG.java:1385)
    at org.testng.TestNG.main(TestNG.java:1354)

I suggest instead of testing strange numbers just test division of everything in range [-10; 10] by 3 and -3, the same for mod.

GreyCat commented 8 months ago

To be honest, I don't see that big of a difference — it merely checks out two use cases (positive and negative) times two sources (constant or value coming from a file).

But, well, [-10; 10] will work the same too. Feel free to implement ;)

Mingun commented 8 months ago

The question what is expected results for -10...10 / 3 and -10...10 / -3? Different languages seems to have different opinions and this opinion is not covered by tests. The result can depend on rounding and it is better to explicitly differentiate, we expect to get floor(a / b), ceil(a / b), or round(a / b) (assuming here that it is float division) and with what sign.

generalmimon commented 8 months ago

The question what is expected results for -10...10 / 3 and -10...10 / -3?

Sometimes I can't decode what you mean by your sentences - is that your question? Did you not see the comment at the top of the KSY spec expr_int_div.ksy that you were pointing to?

expr_int_div.ksy:1-3

# Tests division operation, both positive and negative
# See https://github.com/kaitai-io/kaitai_struct/issues/746
#  => the KS division operation `a / b` should do `floor(a / b)`

I think this states the expected behavior clear enough and it even provides a link to https://github.com/kaitai-io/kaitai_struct/issues/746 with detailed explanation.

The result can depend on rounding

Hypothetically, it probably could if the corresponding translator method of some target in KSC was implemented by someone who doesn't know the correct semantics, but as far as I know, it doesn't in any target language at the moment (i.e. (4 / 3) == (5 / 3) is true in all languages, even though mathematically 4 / 3 = 1.33 and 5 / 3 = 1.67, so if the integer division result depended on rounding, they wouldn't be equal - see also the table in https://github.com/kaitai-io/kaitai_struct/issues/746#issue-613681200 with real results from all target languages).

just test division of everything in range [-10; 10] by 3 and -3

This sounds like the table @dgelessus made (the columns to look at would be floor(i / 3) and "i % 3 behavior 2"), see the expandable section at the bottom of https://github.com/kaitai-io/kaitai_struct/issues/746#issuecomment-624965399 that says:

▸ Table to compare division and modulo behaviors

As @GreyCat said, feel free to improve the test as you see fit, at least we'll have something concrete to discuss.

Mingun commented 8 months ago

Did you not see the comment at the top of the KSY spec expr_int_div.ksy that you were pointing to?

Yes, strangely, but it is true. I really didn't notice it.

The investigation in #746, however, somehow ignored the issue with a negative divisor.

Also for me it is strange that as the test use of strange numbers which result of division it is heavy to calculate manually and that only couple of values from all (incomplete) described table are checked was chosen. I'll prepare PR with exhaustive tests soon. I only hope that it will not stuck as other my (and not only my) PRs here.

kaitai-io / kaitai_struct

Unclear expected values for integer division #1096