Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Buck tests are flaky on non englisch locale #557

Closed
davido opened this issue Dec 7, 2015 · 12 comments
Closed

Buck tests are flaky on non englisch locale #557

davido opened this issue Dec 7, 2015 · 12 comments

Comments

@davido
Copy link
Contributor

davido commented Dec 7, 2015

Trying to run tests on most recent master 4806fdf on two sane platforms (Linux with Java 7 and Mac OS X with Java 8) I'm seeing number of unit test failures:

$ buck test
Not using buckd because watchman isn't installed.
FAILURE com.facebook.buck.util.unit.SizeUnitTest testFormatMegabytes: expected:<1[.]5MB> but was:<1[,]5MB>
FAILURE com.facebook.buck.util.unit.SizeUnitTest testFormatGigabytes: expected:<1[.]5GB> but was:<1[,]5GB>
FAILURE com.facebook.buck.util.unit.SizeUnitTest testFormatTerabytes: expected:<1[.]5TB> but was:<1[,]5TB>
FAILURE com.facebook.buck.util.unit.SizeUnitTest testFormatKilobytes: expected:<-1[.]1KB> but was:<-1[,]1KB>
FAILURE com.facebook.buck.util.TimeFormatTest testFormatForConsole: expected:<  1[.]0s> but was:<  1[,]0s>
FAILURE com.facebook.buck.event.listener.SimpleConsoleEventBusListenerTest testSimpleBuild: expected:<...K FILES...FINISHED 0[.]4s
> but was:<...K FILES...FINISHED 0[,]4s
>
FAILURE com.facebook.buck.event.listener.SuperConsoleEventBusListenerTest testBuildRuleSuspendResumeEvents: 
Expected: <[[-] PROCESSING BUCK FILES...FINISHED 0.0s, [+] BUILDING...0,2s,  |=> IDLE]>
     but: was <[[-] PROCESSING BUCK FILES...FINISHED 0,0s, [+] BUILDING...0,2s,  |=> IDLE]>
FAILURE com.facebook.buck.event.listener.SuperConsoleEventBusListenerTest testSimpleBuildWithProgress: 
Expected: <[[-] PROCESSING BUCK FILES...FINISHED 0,2s, [+] BUILDING...0,1s [0%] (0/10 JOBS, 0 UPDATED, 0.0% CACHE MISS)]>
     but: was <[[-] PROCESSING BUCK FILES...FINISHED 0,2s, [+] BUILDING...0,1s [0%] (0/10 JOBS, 0 UPDATED, 0,0% CACHE MISS)]>

The output above is from OS X 10.10.5 with this locale:

$ locale
LANG="de_DE.UTF-8"
LC_COLLATE="de_DE.UTF-8"
LC_CTYPE="de_DE.UTF-8"
LC_MESSAGES="de_DE.UTF-8"
LC_MONETARY="de_DE.UTF-8"
LC_NUMERIC="de_DE.UTF-8"
LC_TIME="de_DE.UTF-8"
LC_ALL=

The list of failing tests is:

TESTS FAILED: 8 FAILURES
Failed target: //test/com/facebook/buck/event/listener:listener
FAIL com.facebook.buck.event.listener.SimpleConsoleEventBusListenerTest
FAIL com.facebook.buck.event.listener.SuperConsoleEventBusListenerTest
Failed target: //test/com/facebook/buck/util:util
FAIL com.facebook.buck.util.TimeFormatTest
Failed target: //test/com/facebook/buck/util/unit:unit
FAIL com.facebook.buck.util.unit.SizeUnitTest
@davido davido changed the title Buck tests are broken on sane platforms Buck tests are flaky on non englisch locale Dec 7, 2015
@sdwilsh
Copy link
Contributor

sdwilsh commented Dec 7, 2015

These looks like locale-specific failures. @bhamiltoncx, what's the best way to test this type of stuff in a locale-aware way?

@davido
Copy link
Contributor Author

davido commented Dec 7, 2015

According to this comment, it seems like this is a regression, that was already handled once:

https://github.com/facebook/buck/blob/master/test/com/facebook/buck/event/listener/SuperConsoleEventBusListenerTest.java#L110-L124

/**
   * Formats a string with times passed in in seconds.
   *
   * Used to avoid these tests failing if the user's locale doesn't use '.' as the decimal
   * separator, as was the case in https://github.com/facebook/buck/issues/58.
   */
  private static String formatConsoleTimes(String template, Double... time) {
    return String.format(template, (Object[]) FluentIterable.from(ImmutableList.copyOf(time))
        .transform(new Function<Double, String>() {
          @Override
          public String apply(Double input) {
            return timeFormatter.format(input);
          }
        }).toArray(String.class));
  }

And that now in addition to time elapsed, that is handled correctly, at least in this test file, cache hit/missed ratio was added, that is locale specific:

Expected: <0.0% CACHE MISS>
but: was <0,0% CACHE MISS>

@bhamiltoncx
Copy link
Contributor

Yeah, we need to always pass in a Locale object to any method which formats times, dates, amounts, etc. SizeUnit is implicitly using the default Locale but the test is not. Same with SuperConsoleEventBusListener.

I'll fix it.

bhamiltoncx added a commit that referenced this issue Dec 7, 2015
Summary:
This method is unused. Worse, it uses the default
system locale for formatting numbers, so its tests fail
when the system locale uses a decimal comma instead of
a decimal point (say, German).

This deletes the unused method and its tests.

This is a partial fix for #557 .

Test Plan:
Applied patch to force tests into German:
https://gist.github.com/bhamiltoncx/4b6b5785b9d0ba4132e4

Ran `ant java-test -Dtest.class=SizeUnitTest`, confirmed it
failed. Applied this fix, re-ran test, confirmed it passed.

Reviewed By: Coneko

fb-gh-sync-id: 6cba65f
@bhamiltoncx
Copy link
Contributor

The last of the fixes for this issue are out for review internally. I hope to land them today.

bhamiltoncx added a commit that referenced this issue Dec 8, 2015
…matting and parsing

Summary:
Buck currently formats and parses numbers in various ways, most
of which are neither thread-safe nor i18n-safe.

This provides a new wrapper `NumberFormatter` for Java's `NumberFormat`
class which is both thread-safe and i18n-safe.

Instead of using a `ThreadLocal<NumberFormat>` (which leaks memory),
I use a `LoadingCache` which manages a `BlockingQueue<NumberFormat>`.

When a thread needs to format a number, I call `LoadingCache.get(locale)`,
then `blockingQueue.take()` to claim an unused `NumberFormat` for the
calling thread.

I then format the number and put the `NumberFormat` back on the queue.

This is a partial fix for #557 .

Test Plan:
Unit tests included. `ant java-test -Dtest.class=NumberFormatterTest`
and `buck test //test/com/facebook/buck/i18n:i18n`.

Reviewed By: sdwilsh

fb-gh-sync-id: 3daa33a
@bhamiltoncx
Copy link
Contributor

Apologies for the delay. The fixes were reviewed last night and are landing now.

bhamiltoncx added a commit that referenced this issue Dec 9, 2015
Summary:
Previously, `SuperConsoleEventBusListener` and `SimpleConsoleEventBusListener`
hard-coded the default locale of the running system. That meant timestamps
were formattted like `0.0s` for US English and `0,0s` for German.

Unfortunately, the tests had a mix of default locale and hard-coded US English
timestamps. The tests all passed in our environment, but failed for people
testing Buck in a non-English environment.

This fixes the issue by passing a `Locale` down to `SimpleConsoleEventBusListener`
/ `SuperConsoleEventBusListener` and passing that to a `NumberFormatter` which
deals with formatting timestamps appropriately for the locale.

To keep this diff small, I didn't delete `SuperConsoleEventBusListenerTest.formatConsoleTimes()`,
even though it's unnecessary now. In the next diff, I will delete the test utility method.

Partial fix for #557 .

Test Plan:
New unit test added. `ant java-test -Dtest.class=SuperConsoleEventBusListenerTest`,
`ant java-test -Dtest.class=SimpleConsoleEventBusListenerTest`.

Applied this patch to repo to hard-code German language as system default in `ant java-test`:

https://gist.github.com/bhamiltoncx/4b6b5785b9d0ba4132e4

Ran tests with `ant java-test -Dtest.class=SuperConsoleEventBusListenerTest` and
`ant java-test -Dtest.class=SimpleConsoleEventBusListenerTest`, confirmed
they both failed.

Applied patch, re-ran tests, confirmed they both succeeded.

Reviewed By: sdwilsh

fb-gh-sync-id: 2339def
bhamiltoncx added a commit that referenced this issue Dec 9, 2015
Summary:
This is the last test which fails in non-US English locales.

The only places it's used are from `TestResultSummary` and `TestCaseSummary`.

Refactoring `TestResultSummary` to support multiple locales is pretty painful, so I hard-coded
that class to US English.

Refactoring `TestCaseSummary` isn't as bad, so I'll separately send out a diff to fix that. Temporarily, this diff hard-codes it to US English as well.

Partial fix for #557

Test Plan: `ant java-test -Dtest.class=TimeFormatTest`

Reviewed By: k21

fb-gh-sync-id: 78752f4
@davido
Copy link
Contributor Author

davido commented Dec 9, 2015

Thanks. Trying again on most recent master (ec75fa1) still some tests are failing here:

$ buck test
Shutting down nailgun server...
Using watchman.
Waiting for Watchman command [/home/davido/bin/watchman version]...
Timed out after 10000 ms waiting for Watchman command [/home/davido/bin/watchman version]. Disabling Watchman.
FAILURE com.facebook.buck.event.listener.BuildThreadStateRendererTest withMissingInformation: 
Expected: is <[ |=> //:target3...  2.6s (checking local cache),  |=> IDLE,  |=> IDLE]>
     but: was <[ |=> //:target3...  2,6s (checking local cache),  |=> IDLE,  |=> IDLE]>
FAILURE com.facebook.buck.event.listener.BuildThreadStateRendererTest commonCase: 
Expected: is <[ |=> //:target2...  4.4s (running step A[2.7s]),  |=> //:target3...  2.6s (checking local cache),  |=> //:target1...  3.3s (checking local cache),  |=> IDLE,  |=> //:target4...  1.2s (running step B[0.5s])]>
     but: was <[ |=> //:target2...  4,4s (running step A[2,7s]),  |=> //:target3...  2,6s (checking local cache),  |=> //:target1...  3,3s (checking local cache),  |=> IDLE,  |=> //:target4...  1,2s (running step B[0,5s])]>
FAILURE com.facebook.buck.event.listener.TestThreadStateRendererTest withMissingInformation: 
Expected: is <[ |=> //:target3...  2.6s,  |=> //:target1...  3.3s,  |=> IDLE,  |=> IDLE]>
     but: was <[ |=> //:target3...  2,6s,  |=> //:target1...  3,3s,  |=> IDLE,  |=> IDLE]>
FAILURE com.facebook.buck.event.listener.TestThreadStateRendererTest commonCase: 
Expected: is <[ |=> //:target2...  4.4s (running step A[2.7s]),  |=> //:target3...  2.6s (running Test A[2.6s]),  |=> //:target1...  3.3s,  |=> IDLE,  |=> //:target4...  1.2s (running Test B[0.4s])]>
     but: was <[ |=> //:target2...  4,4s (running step A[2,7s]),  |=> //:target3...  2,6s (running Test A[2,6s]),  |=> //:target1...  3,3s,  |=> IDLE,  |=> //:target4...  1,2s (running Test B[0,4s])]>
FAILURE com.facebook.buck.python.PythonBinaryIntegrationTest nativeLibsEnvVarIsPreserved[INPLACE]: 
Expected: "None"
     but: was "/usr/lib64/mpi/gcc/openmpi/lib64"
FAILURE com.facebook.buck.python.PythonBinaryIntegrationTest nativeLibsEnvVarIsPreserved[STANDALONE]: 
Expected: "None"
     but: was "/usr/lib64/mpi/gcc/openmpi/lib64"

Here is the list of failing tests:

Failed target: //test/com/facebook/buck/event/listener:listener
FAIL com.facebook.buck.event.listener.BuildThreadStateRendererTest
FAIL com.facebook.buck.event.listener.TestThreadStateRendererTest
Failed target: //test/com/facebook/buck/python:python
FAIL com.facebook.buck.python.PythonBinaryIntegrationTest

Here is more verbose log:

http://paste.openstack.org/show/481381.

@k21
Copy link
Contributor

k21 commented Dec 9, 2015

The *ThreadStateRendererTest failures are my fault, I have just added them before the @bhamiltoncx's fix and they are using the system default locale. I will make sure to fix them soon.

@k21 k21 reopened this Dec 9, 2015
@davido
Copy link
Contributor Author

davido commented Dec 9, 2015

Thx.

@bhamiltoncx
Copy link
Contributor

Thanks @k21. I should have re-run all the tests in German (I forgot to do it after your diff).

@davido, the PythonBinaryIntegrationTest failure appears to be a separate problem.

@davido
Copy link
Contributor Author

davido commented Dec 10, 2015

@bhamiltoncx Thanks for the analysis. I filed new issue to track this failure: #565. Does it work for you?

I'm a bit confused. CI is broken, I cannot get test passed on my Linux machine and getting negative reviews because my PR #558 broke some tests. But how should I be able to track down this breakage, when tests are inherently broken on most recent master (ec75fa1) anyway? Or this is something that environment dependent?

@davido
Copy link
Contributor Author

davido commented Dec 10, 2015

Fixed in: [1].

@davido
Copy link
Contributor Author

davido commented Dec 10, 2015

Fixed by: 447e59a

@davido davido closed this as completed Dec 10, 2015
shs96c pushed a commit to shs96c/buck that referenced this issue Dec 26, 2015
Summary:
This method is unused. Worse, it uses the default
system locale for formatting numbers, so its tests fail
when the system locale uses a decimal comma instead of
a decimal point (say, German).

This deletes the unused method and its tests.

This is a partial fix for facebook#557 .

Test Plan:
Applied patch to force tests into German:
https://gist.github.com/bhamiltoncx/4b6b5785b9d0ba4132e4

Ran `ant java-test -Dtest.class=SizeUnitTest`, confirmed it
failed. Applied this fix, re-ran test, confirmed it passed.

Reviewed By: Coneko

fb-gh-sync-id: 6cba65f
shs96c pushed a commit to shs96c/buck that referenced this issue Dec 26, 2015
…matting and parsing

Summary:
Buck currently formats and parses numbers in various ways, most
of which are neither thread-safe nor i18n-safe.

This provides a new wrapper `NumberFormatter` for Java's `NumberFormat`
class which is both thread-safe and i18n-safe.

Instead of using a `ThreadLocal<NumberFormat>` (which leaks memory),
I use a `LoadingCache` which manages a `BlockingQueue<NumberFormat>`.

When a thread needs to format a number, I call `LoadingCache.get(locale)`,
then `blockingQueue.take()` to claim an unused `NumberFormat` for the
calling thread.

I then format the number and put the `NumberFormat` back on the queue.

This is a partial fix for facebook#557 .

Test Plan:
Unit tests included. `ant java-test -Dtest.class=NumberFormatterTest`
and `buck test //test/com/facebook/buck/i18n:i18n`.

Reviewed By: sdwilsh

fb-gh-sync-id: 3daa33a
shs96c pushed a commit to shs96c/buck that referenced this issue Dec 26, 2015
Summary:
Previously, `SuperConsoleEventBusListener` and `SimpleConsoleEventBusListener`
hard-coded the default locale of the running system. That meant timestamps
were formattted like `0.0s` for US English and `0,0s` for German.

Unfortunately, the tests had a mix of default locale and hard-coded US English
timestamps. The tests all passed in our environment, but failed for people
testing Buck in a non-English environment.

This fixes the issue by passing a `Locale` down to `SimpleConsoleEventBusListener`
/ `SuperConsoleEventBusListener` and passing that to a `NumberFormatter` which
deals with formatting timestamps appropriately for the locale.

To keep this diff small, I didn't delete `SuperConsoleEventBusListenerTest.formatConsoleTimes()`,
even though it's unnecessary now. In the next diff, I will delete the test utility method.

Partial fix for facebook#557 .

Test Plan:
New unit test added. `ant java-test -Dtest.class=SuperConsoleEventBusListenerTest`,
`ant java-test -Dtest.class=SimpleConsoleEventBusListenerTest`.

Applied this patch to repo to hard-code German language as system default in `ant java-test`:

https://gist.github.com/bhamiltoncx/4b6b5785b9d0ba4132e4

Ran tests with `ant java-test -Dtest.class=SuperConsoleEventBusListenerTest` and
`ant java-test -Dtest.class=SimpleConsoleEventBusListenerTest`, confirmed
they both failed.

Applied patch, re-ran tests, confirmed they both succeeded.

Reviewed By: sdwilsh

fb-gh-sync-id: 2339def
shs96c pushed a commit to shs96c/buck that referenced this issue Dec 26, 2015
Summary:
This is the last test which fails in non-US English locales.

The only places it's used are from `TestResultSummary` and `TestCaseSummary`.

Refactoring `TestResultSummary` to support multiple locales is pretty painful, so I hard-coded
that class to US English.

Refactoring `TestCaseSummary` isn't as bad, so I'll separately send out a diff to fix that. Temporarily, this diff hard-codes it to US English as well.

Partial fix for facebook#557

Test Plan: `ant java-test -Dtest.class=TimeFormatTest`

Reviewed By: k21

fb-gh-sync-id: 78752f4
shs96c pushed a commit to shs96c/buck that referenced this issue Dec 26, 2015
Summary:
This should be the last of the fixes to ensure
our tests can control the locale under which they run.

This passes `Locale` down to `TestCaseSummary` and `TestResultFormatter`
so they no longer implicitly use the system locale.

That allows us to ensure our tests pass when run in a non-English
locale.

Fixes facebook#557

Test Plan:
New test added.

  ant java-test -Dtest.class=TestResultFormatterTest
  ant java-test -Dtest.class=TestRunTest

Reviewed By: k21

fb-gh-sync-id: 28f2565
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants