Fix JUnit tests failing for non-US locales and timezones

[spannm]

Small patch to fix three failing unit tests by a) explicitly setting Locale to US in two tests and b) removing an explicit timezone in one test.

I work under a DE (German) locale in the CET (UTC+01:00) timezone.

Overview of test failures before patch:

Failed tests: 
  NumberFormatterTest.testDecimalFormat:92 expected:<9874539485972[.]2342342234234> but was:<9874539485972[,]2342342234234>
  NumberFormatterTest.testFloatFormat:67 expected:<8949[.]847> but was:<8949[,]847>
  NumberFormatterTest.testDoubleFormat:40 expected:<8949[.]84737284944> but was:<8949[,]84737284944>
  DatabaseTest.testAncientDates:732 expected:<[1582-10-15, 1582-10-14, 1492-01-10, 1392-01-10]> but was:<[1582-10-14, 1582-10-13, 1492-01-09, 1392-01-09]>
  DefaultFunctionsTest.testFinancialFuncs:780->assertEval:858 expected:<-9.57859403981306> but was:<-9,57859403981306>
  DefaultFunctionsTest.testFormat:271->assertEval:858 expected:<12345.6789> but was:<12345,6789>

Tests run: 193, Failures: 6, Errors: 0, Skipped: 0

[jahlborn]

can you explain why you removed the explicit timezone? the point of the explicit timezone in the unit test is to avoid failures when running in a different timezone.

[spannm]

DatabaseTest.testAncientDates fails without my patch. Failure details:

Tests run: 28, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.93 sec <<< FAILURE! - in com.healthmarketscience.jackcess.DatabaseTest
testAncientDates(com.healthmarketscience.jackcess.DatabaseTest)  Time elapsed: 0.028 sec  <<< FAILURE!
junit.framework.AssertionFailedError: expected:<[1582-10-15, 1582-10-14, 1492-01-10, 1392-01-10]> but was:<[1582-10-14, 1582-10-13, 1492-01-09, 1392-01-09]>
	at junit.framework.Assert.fail(Assert.java:57)
	at junit.framework.Assert.failNotEquals(Assert.java:329)
	at junit.framework.Assert.assertEquals(Assert.java:78)
	at junit.framework.Assert.assertEquals(Assert.java:86)
	at junit.framework.TestCase.assertEquals(TestCase.java:246)
	at com.healthmarketscience.jackcess.DatabaseTest.testAncientDates(DatabaseTest.java:732)

[jahlborn]

DatabaseTest.testAncientDates fails without my patch. Failure details:

Tests run: 28, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.93 sec <<< FAILURE! - in com.healthmarketscience.jackcess.DatabaseTest
testAncientDates(com.healthmarketscience.jackcess.DatabaseTest)  Time elapsed: 0.028 sec  <<< FAILURE!
junit.framework.AssertionFailedError: expected:<[1582-10-15, 1582-10-14, 1492-01-10, 1392-01-10]> but was:<[1582-10-14, 1582-10-13, 1492-01-09, 1392-01-09]>
	at junit.framework.Assert.fail(Assert.java:57)
	at junit.framework.Assert.failNotEquals(Assert.java:329)
	at junit.framework.Assert.assertEquals(Assert.java:78)
	at junit.framework.Assert.assertEquals(Assert.java:86)
	at junit.framework.TestCase.assertEquals(TestCase.java:246)
	at com.healthmarketscience.jackcess.DatabaseTest.testAncientDates(DatabaseTest.java:732)

that would seem to indicate a bug somewhere in jackcess. using a fixed timezone should return the same dates everywhere.

[spannm]

Hi @jahlborn,

I've split test case testAncientDates into two methods There are really two concerns, first creates dbs and inserts on the fly, while second runs tests on a canned db from src/test/data.

When accessing test db oldDatesV2007.accdb the test did not set the time zone on the database object while it should. The default is local timezone, which in your case is "America/New_York" I assume. Therefore for you the test is green, while it fails in other timezones (depending on the offset to "America/New_York").

I have added (line 734):

db.setTimeZone(tz); // explicitly set database time zone

Tests are green now in either timezone. Maven command lines I've used:

mvn clean test -Duser.timezone="Europe/Berlin" -Dtest=DatabaseTest.java
mvn clean test -Duser.timezone="America/New_York" -Dtest=DatabaseTest.java

Updated pr and force pushed. Please take another look - thx.

[jahlborn]

Updated pr and force pushed. Please take another look - thx.

ah, that looks much better, thanks!

[jahlborn]

@spannm i incorporated those changes, although i went a slightly different direction with the locale settings (i was concerned that messing with the default locale might now work reliably in a multi-threaded setting). let me know if this works for you, thanks.

[spannm]

Hi @jahlborn,

setting the locale to US for all tests limits the value of the library's test suit IMO. <argLine>-Duser.language=en -Duser.region=US</argLine> Tests now run under US locale no matter where and how (around the world) they are run. Non-US locale-related issues would go undetected. I would advise against it, and manipulate locale in tests only where absolutely required, and in every such case where locale != default we should know why we do so. JM2C

[spannm]

different direction with the locale settings (i was concerned that messing with the default locale might now work reliably in a multi-threaded setting)

You're concerned about the private static Locale in the two test cases? How is JUnit 4 test parallelization used in jackcess? JUnit guarantees that BeforeClass, all the tests (random order), and AfterClass are run in exactly this order. The only concern would be tests in other classes executed in parallel at the same time while temporarily the locale was changed. Not sure we have a problem here and would like to see a unit test fail first, but I agree manipulating locale in tests is not great. We could limit the locale manipulation to only the failing test methods. However then we must make sure that locale is restored even in case of test failures (e.g. try/catch with finally block).

Btw are you open to upgrading to JUnit 5? Your project would benefit from ParameterizedTest There is also a lot more granularity for parallel test execution with annotations such as @Execution(ExecutionMode.SAME_THREAD) or @Execution(ExecutionMode.CONCURRENT)

[jahlborn]

different direction with the locale settings (i was concerned that messing with the default locale might now work reliably in a multi-threaded setting)

You're concerned about the private static Locale in the two test cases? How is JUnit 4 test parallelization used in jackcess? JUnit guarantees that BeforeClass, all the tests (random order), and AfterClass are run in exactly this order. The only concern would be tests in other classes executed in parallel at the same time while temporarily the locale was changed. Not sure we have a problem here and would like to see a unit test fail first, but I agree manipulating locale in tests is not great. We could limit the locale manipulation to only the failing test methods. However then we must make sure that locale is restored even in case of test failures (e.g. try/catch with finally block).

the surefire plugin is running with 2 threads, so yes, two different tests could be running at the same time which could mean a test is running while the jvm's locale is changing. is there a problem with the way i set the locale in the pom file?

Btw are you open to upgrading to JUnit 5? Your project would benefit from ParameterizedTest There is also a lot more granularity for parallel test execution with annotations such as @Execution(ExecutionMode.SAME_THREAD) or @Execution(ExecutionMode.CONCURRENT)

someone else posted a PR to upgrade to junit 5. when i incorporated the changes, the tests ran 50% longer. since there isn't really any functional benefit, i left the code on junit 4.

[spannm]

is there a problem with the way i set the locale in the pom file?

In a technical sense no. But tests now run under US locale everywhere and always. This limits the value of the test suite a lot. The tests should work under any locale. Non-US locale-related issues would go undetected.

[jahlborn]

is there a problem with the way i set the locale in the pom file?

In a technical sense no. But tests now run under US locale everywhere and always. This limits the value of the test suite a lot. The tests should work under any locale. Non-US locale-related issues would go undetected.

yeah that's a fair point. i was debating that when i made the change. thinking about this some more, the tests that are failing are related to expression parsing. i'm guessing the expression parsing logic is already "en_US" specific. maybe that jackcess logic should be using a hard coded locale.

[spannm]

I'll try another approach how we can resolve this neatly. I will update the pr and let you know tomorrow.

[spannm]

Hi @jahlborn,

I've taken another spin on this code. It was a bit of work but now we have a solution that I am happy with, and I hope you, too

• NumberFormatter is now Locale aware

  • in NumberFormatterTest an instance is created with an explicit US locale

• DefaultFunctionsTest

  • inlined assertEval to have control over the assertEquals signatures (i.e. double comparisons) and for simplicity
  • test testFormat split into several test methods
  • Functions under test that return String containing formatted numbers now have expected results created using String.format (which is locale-aware)
  • Removed all explicit conversions to String (via CStr) in testFinancialFuncs. Now asserting against numeric values returned by the function (the point of these tests was not to test CStr, this is tested elsewhere).

Tests are green:

mvn clean test -Duser.timezone="Europe/Berlin" -Duser.region=DE -Duser.language=de
mvn clean test -Duser.timezone="America/New_York" -Duser.region=US -Duser.language=en
mvn clean test -Duser.timezone="Asia/Tokyo" -Duser.region=JP -Duser.language=ja

My PR is updated accordingly.

If you merge please make sure to revert this commit:

ensure unit tests run reliably in different locales
git-svn-id: https://svn.code.sf.net/p/jackcess/code/jackcess/trunk@1405 f203690c-595d-4dc9-a70b-905162fa7fd2

cheers Markus

[jahlbornG]

So I agree that NumberFormatter is probably the issue. I was looking at it a bit yesterday. I'm not really sure why I implemented NumberFormatter the way I did. I'm actually thinking that it would make more sense to make it part of the LocaleContext. I may take a crack at this tomorrow.

[spannm]

The pull request is solid and solves the problem at hand IMO. How about merging the PR and taking a spin at a possible NumberFormatter rewrite or refactoring in a separate step? Have a good week-end!

[jahlborn]

The pull request is solid and solves the problem at hand IMO. How about merging the PR and taking a spin at a possible NumberFormatter rewrite or refactoring in a separate step? Have a good week-end!

the problem is that it doesn't really solve the problem that NumberFormatter doesn't honor the locale of the db. It just fixes the unit test. but my simple change to the pom also fixes the unit test.

[jahlborn]

I updated NumberFormatter to be locale aware and changed the formatting to go through the NumericConfig attached to the current LocaleContext. This makes the number formatting follow the currently configured db. I removed the changes in the pom to use a specific locale for unit tests. i believe this should work in your locale now.

[spannm]

Looks like no part at all of my PR actually made it into master. Not even the improvements to DefaultFunctionsTest.

[jahlbornG]

I incorporated your fixes for the time zone issue in the unit test. Can you explain more what the purpose of the changes to the DefaultFuntionsTest were? The tests weren't failing, so it wasn't clear to me what the changes accomplished

[spannm]

Test DefaultFuntionsTest.testFinancialFuncs was failing due to Locale (due to the usage of CStr to be precise). The conversion to CStr is out of place here IMO since numerical functions are being tested. But never mind, I'll leave it up to you whether you take theses changes or not.

[jahlborn]

Test DefaultFuntionsTest.testFinancialFuncs was failing due to Locale (due to the usage of CStr to be precise).

as far as i can tell, all tests are now succeeding even when i change the locale. is that true for you as well?

[spannm]

as far as i can tell, all tests are now succeeding even when i change the locale. is that true for you as well?

yup, tests are green under different locales. Tried a couple

[jahlborn]

incorporated the relevant changes.