Lucene - Core
  1. Lucene - Core
  2. LUCENE-4020

Tests may not be repeatable across different platforms/ JVMs due to different locale/ timezone being picked.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is because the source array can be/ is different for each system/ JVM. So this pick is not repeatable for example:

        /** 
         * Return a random Locale from the available locales on the system.
         */
        public static Locale randomLocale(Random random) {
          Locale locales[] = Locale.getAvailableLocales();
          return locales[random.nextInt(locales.length)];
        }
      

      I don't think there is much we can do to make it repeatable (other than maybe enforcing locale using system property).

      1. LUCENE-4020.diff
        1 kB
        Dawid Weiss
      2. LUCENE-4020.patch
        8 kB
        Dawid Weiss

        Activity

        Hide
        Dawid Weiss added a comment -

        I think the "reproduce with" line should explicitly state the timezone and locale picked for the test. Granted, certain combinations will be invalid on other JVMs/ systems, but at least it's explicit what should be used?

        Show
        Dawid Weiss added a comment - I think the "reproduce with" line should explicitly state the timezone and locale picked for the test. Granted, certain combinations will be invalid on other JVMs/ systems, but at least it's explicit what should be used?
        Hide
        Dawid Weiss added a comment -

        Suggested patch.

        Show
        Dawid Weiss added a comment - Suggested patch.
        Hide
        Dawid Weiss added a comment -

        Oops, sorry, wrong thinking. It has to be the actual Locale, not the passed property.

        Show
        Dawid Weiss added a comment - Oops, sorry, wrong thinking. It has to be the actual Locale, not the passed property.
        Hide
        Dawid Weiss added a comment -

        Ok, this time it's right.

        Show
        Dawid Weiss added a comment - Ok, this time it's right.
        Hide
        Robert Muir added a comment -

        don't we need to also take care to still consume the same number of random bits
        when these params are specified?

        Show
        Robert Muir added a comment - don't we need to also take care to still consume the same number of random bits when these params are specified?
        Hide
        Dawid Weiss added a comment -

        Good point, yes it should be identical. I'll update in the evening.

        Show
        Dawid Weiss added a comment - Good point, yes it should be identical. I'll update in the evening.
        Hide
        Dawid Weiss added a comment -

        Updated patch. Had to make fields non-final to make tests of this possible.

        Show
        Dawid Weiss added a comment - Updated patch. Had to make fields non-final to make tests of this possible.
        Hide
        Robert Muir added a comment -

        +1 looks great

        Show
        Robert Muir added a comment - +1 looks great
        Hide
        Dawid Weiss added a comment -

        It'll fail on 1.7 and those darn script variant locales should they be picked for the test... The first time it does, I'll think what to do about it.

        Show
        Dawid Weiss added a comment - It'll fail on 1.7 and those darn script variant locales should they be picked for the test... The first time it does, I'll think what to do about it.
        Hide
        Hoss Man added a comment -

        straw man suggestion:

        change the impl of randomLocale(Random) to ensure that it only returns Locale objects that can be round tripped...

        public static Locale randomLocale(final Random outerRandom) {
          // consume a fixed amount of randomness from outerRandom
          final Random random = new Random(outerRandom.nextLong());
          
          Locale locales[] = Locale.getAvailableLocales();
          Locale result = null;
          for (int i = 0; i < 100; i++) {  
            Locale candidate = locales[random.nextInt(locales.length)];
            try {
               Locale roundTrip = localeForName(candidate.toString());
               if (candidate.equals(roundTrip)) {
                 result = candidate;
                 break;
               }
            } catch (Exception e) {
               // :NOOP: ... go around again
            }
          }
          if (null == result) {
            throw new YourJvmIsFuckedException("Gave up trying to pick a random Locale")
          }
          return result;
        }
        
        Show
        Hoss Man added a comment - straw man suggestion: change the impl of randomLocale(Random) to ensure that it only returns Locale objects that can be round tripped... public static Locale randomLocale( final Random outerRandom) { // consume a fixed amount of randomness from outerRandom final Random random = new Random(outerRandom.nextLong()); Locale locales[] = Locale.getAvailableLocales(); Locale result = null ; for ( int i = 0; i < 100; i++) { Locale candidate = locales[random.nextInt(locales.length)]; try { Locale roundTrip = localeForName(candidate.toString()); if (candidate.equals(roundTrip)) { result = candidate; break ; } } catch (Exception e) { // :NOOP: ... go around again } } if ( null == result) { throw new YourJvmIsFuckedException( "Gave up trying to pick a random Locale" ) } return result; }
        Hide
        Robert Muir added a comment -

        but your jvm isnt fucked, and we should test the new java 7 locales.

        Its just this idea of roundtripping without using anything but java7 BCP47 apis is fucked.

        Show
        Robert Muir added a comment - but your jvm isnt fucked, and we should test the new java 7 locales. Its just this idea of roundtripping without using anything but java7 BCP47 apis is fucked.
        Hide
        Hoss Man added a comment -

        so the exception name isn't great ... but the point is there should be a decent number of Locales that work, and we should be able to try N times to find one, nad if we really don't then give up because we can't run a reproducible test.

        (or hell: don't give up .. try 100 times and if we still don't get one that round trips just use whatever we did get and let the user deal with the fact that they can't pass that locale back in to reproduce if it fails)

        Show
        Hoss Man added a comment - so the exception name isn't great ... but the point is there should be a decent number of Locales that work, and we should be able to try N times to find one, nad if we really don't then give up because we can't run a reproducible test. (or hell: don't give up .. try 100 times and if we still don't get one that round trips just use whatever we did get and let the user deal with the fact that they can't pass that locale back in to reproduce if it fails)
        Hide
        Robert Muir added a comment -

        I can make them all work.

        Show
        Robert Muir added a comment - I can make them all work.
        Hide
        Robert Muir added a comment -

        Patch is on 4021 for round trip parsing (in 6 and 7). It should also work across 6 and 7,
        e.g. in on java6 we emit th_TH_TH, on java7 th_TH_TH_#u-nu-thai, but both are constructed as th_TH_TH.

        This is ok because of the backwards mechanism (Special Cases) listed in http://docs.oracle.com/javase/7/docs/api/java/util/Locale.htm

        Show
        Robert Muir added a comment - Patch is on 4021 for round trip parsing (in 6 and 7). It should also work across 6 and 7, e.g. in on java6 we emit th_TH_TH, on java7 th_TH_TH_#u-nu-thai, but both are constructed as th_TH_TH. This is ok because of the backwards mechanism (Special Cases) listed in http://docs.oracle.com/javase/7/docs/api/java/util/Locale.htm
        Hide
        Dawid Weiss added a comment -

        Fantastic, thanks.

        Show
        Dawid Weiss added a comment - Fantastic, thanks.

          People

          • Assignee:
            Dawid Weiss
            Reporter:
            Dawid Weiss
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development