OpusStatistics test failure (Possibly Locale related)

[FearThe1337]

Hi,

When attempting to build:

expected:<00:00:00[.]02> but was:<00:00:00[,]02>

I'm assuming it's because of a system locale setting. Full stack trace:

Running org.gagravarr.opus.TestOpusStatistics
Tests run: 2, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.001 sec <<< FAILURE!
testReadInfo09(org.gagravarr.opus.TestOpusStatistics)  Time elapsed: 0 sec  <<< FAILURE!
junit.framework.ComparisonFailure: expected:<00:00:00[.]02> but was:<00:00:00[,]02>
        at junit.framework.Assert.assertEquals(Assert.java:100)
        at junit.framework.Assert.assertEquals(Assert.java:107)
        at junit.framework.TestCase.assertEquals(TestCase.java:269)
        at org.gagravarr.opus.TestOpusStatistics.testReadInfo09(TestOpusStatistics.java:67)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at junit.framework.TestCase.runTest(TestCase.java:176)
        at junit.framework.TestCase.runBare(TestCase.java:141)
        at junit.framework.TestResult$1.protect(TestResult.java:122)
        at junit.framework.TestResult.runProtected(TestResult.java:142)
        at junit.framework.TestResult.run(TestResult.java:125)
        at junit.framework.TestCase.run(TestCase.java:129)
        at junit.framework.TestSuite.runTest(TestSuite.java:255)
        at junit.framework.TestSuite.run(TestSuite.java:250)
        at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:84)
        at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:252)
        at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:141)
        at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:112)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:189)
        at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:165)
        at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:85)
        at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:115)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:75)

[Gagravarr]

I'd suggest you just move to the UK - it'll fix all your timezone and locale bugs ;-)

I'm currently trying to get the last of the FLAC audio length bits done. When that's sorted, I'll try turning on the Forbidden APIs checks, and fix up all the things that flags. Hopefully one of those should fix this locale assumption bug

[croger42]

It is not the only test in this case: all tests with assertions on duration could fail because the String.format method is locale-aware. One quickfix is to use String.format on the left side of the assertion too. And I'm not sure forcing the Locale is a good idea.
Quickfix in pull request #28

[andrm]

As the original author of the test, I'm sorry for this locale dependency. Quickfix looks good to me.

[Gagravarr]

I'm in two minds about what's the "least surprising" behaviour here

If we want to keep it returning mostly-local formatted time, then @croger42's fix to the unit tests looks correct. (I'd be minded to put in a helper method to shorten the code it a bit, but that's it!)

However... In Apache Tika we've tended to force all our formatting stuff to be locale-independant by default, which would mean changing OggAudioStatistics to use Locale.ROOT or similar when doing the format, so it's always the same. That's also what adding the Forbidden API checks (long forgotten about, sorry!) would lean towards, as that generally prevents you from using default-locale methods implicitly

As apparently the main two non-UK / non-US users of the method, what do you both think? :)

[andrm]

To me the root problem is that getDuration() uses the default-locale. Suggestion: let add/change it to getDuration(Locale l) which uses the one the user asks for. Tests can force UK locale for instance.

[Gagravarr]

@andrm I've hopefully done that in d82dbda , does the test now work correctly (and expectedly!) for you?

[andrm]

Works well, tested it with several locale settings.

Can you please look at 989a72a and andrm@c540c85 ? I think that's better and we keep backwards compatibility. Sorry for not doing a pull request, my repo is messed up.

[Gagravarr]

Great, thanks for everyone's help on this, and sorry it took so long to solve!