qos-ch / reload4j

reload4j is a drop-in replacement for log4j 1.2.17
Apache License 2.0
148 stars 22 forks source link

Localization issues in tests #38

Closed fipro78 closed 2 years ago

fipro78 commented 2 years ago

When I try to execute the build locally I get the following error:

[ERROR] Failures:
[ERROR]   TestLogMF.testDebugDouble:458 expected:<Iteration 3[,]142> but was:<Iteration 3[.]142>
[ERROR]   TestLogMF.testDebugFloat:446 expected:<Iteration 3[,]142> but was:<Iteration 3[.]142>
[ERROR]   TestLogMF.testInfoDouble:702 expected:<Iteration 3[,]142> but was:<Iteration 3[.]142>
[ERROR]   TestLogMF.testInfoFloat:690 expected:<Iteration 3[,]142> but was:<Iteration 3[.]142>
[ERROR]   TestLogMF.testTraceDouble:249 expected:<Iteration 3[,]14> but was:<Iteration 3[.]14>
[ERROR]   TestLogMF.testTraceFloat:235 expected:<Iteration 3[,]14> but was:<Iteration 3[.]14>

I suppose this is a localization issue in the test case.

ceki commented 2 years ago

Given the commits from earlier and your comment https://github.com/qos-ch/reload4j/issues/27#issuecomment-1031521391, is this issue still relevant?

fipro78 commented 2 years ago

I executed the build with -DskipTests=true

Just tried again after pulling the latest changes and the issue still occurs.

IMHO the issue is that the test uses NumberFormat.getInstance(); without a Locale parameter. So it uses the current default locale. Not sure if the trace() method takes the Locale into account. But there seems to be a mismatch on my machine.

ceki commented 2 years ago

I see. Hopefully fixed in new commit ad82fa07bc40b

fipro78 commented 2 years ago

@ceki Unfortunately not. But actually it is not an issue in the code, it is an issue in the tests. The tests are not using the same format with Locale as the code, so they do not match.

I got it working locally now and will prepare a PR.

fipro78 commented 2 years ago

@ceki I also thought that your change was not necessary and should be reverted to ensure that no side effect happens. But I didn't want to change it without your oppinion. :)

Just pulled the latest master and successfully executed the build. Tests are all passing now.