Lucene - Core
  1. Lucene - Core
  2. LUCENE-4667

Change TestRandomChains to replace the list of broken classes by a list of broken constructors

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Some classes are currently in the list of bad apples although only one constructor is broken. For example, LimitTokenCountFilter has an option to consume the whole stream.

      1. LUCENE-4667.patch
        11 kB
        Adrien Grand
      2. LUCENE-4667.patch
        11 kB
        Adrien Grand
      3. LUCENE-4667.patch
        4 kB
        Adrien Grand

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          +1

          Show
          Robert Muir added a comment - +1
          Hide
          Adrien Grand added a comment -

          Patch.

          Show
          Adrien Grand added a comment - Patch.
          Hide
          Uwe Schindler added a comment -

          Looks fine. I would prfer to use IdentityHashMap instead of HashMap, so it is consistent with the remaining logic. Classes and Constructors should be compared with identity. I would also make all constructors in the Map with the ALWAYS predicate to be not added to the array lists from the beginning.

          Show
          Uwe Schindler added a comment - Looks fine. I would prfer to use IdentityHashMap instead of HashMap, so it is consistent with the remaining logic. Classes and Constructors should be compared with identity. I would also make all constructors in the Map with the ALWAYS predicate to be not added to the array lists from the beginning.
          Hide
          Adrien Grand added a comment -

          The test failed when I used an IdentityHashMap. Did I miss something or can't constructors be compared using ==?

          Show
          Adrien Grand added a comment - The test failed when I used an IdentityHashMap. Did I miss something or can't constructors be compared using ==?
          Hide
          Adrien Grand added a comment -

          New patch that adds exceptions to TrimFilter and TypeTokenFilter as well and uses a constructor map for all components, following Uwe's advice.

          Show
          Adrien Grand added a comment - New patch that adds exceptions to TrimFilter and TypeTokenFilter as well and uses a constructor map for all components, following Uwe's advice.
          Hide
          Uwe Schindler added a comment -

          Maybe that's the case! Sorry. I was expecting that constructors are singletons like classes. HashMap is fine then.

          In my opinion, I think maybe the whole Predicate approach is too much detailed? I would just match on the constructor itsself and would disallow it completeley (without looking into actual parameters). Just exclude the constructor in the beforeClass() method when populating the lists.

          If you want to keep the predicate approach, i would exclude all broken construcors with the ALWAYS predicate in beforeClass(), so it never tries to use the constructor at all (because its no longer in the list).

          Show
          Uwe Schindler added a comment - Maybe that's the case! Sorry. I was expecting that constructors are singletons like classes. HashMap is fine then. In my opinion, I think maybe the whole Predicate approach is too much detailed? I would just match on the constructor itsself and would disallow it completeley (without looking into actual parameters). Just exclude the constructor in the beforeClass() method when populating the lists. If you want to keep the predicate approach, i would exclude all broken construcors with the ALWAYS predicate in beforeClass(), so it never tries to use the constructor at all (because its no longer in the list).
          Hide
          Adrien Grand added a comment -

          Maybe that's the case! Sorry. I was expecting that constructors are singletons like classes.

          No problem, I had the same expectation and was a little disappointed to see that it didn't work!

          I think maybe the whole Predicate approach is too much detailed?

          I think it's worth exluding with a predicate: for example this allows to test random chains with LimitTokenCountFilter(consumeAllTokens=true) (when consumeAllTokens=false, this filter is broken).

          I would exclude all broken construcors with the ALWAYS predicate in beforeClass()

          Sounds good, I updated the patch.

          Show
          Adrien Grand added a comment - Maybe that's the case! Sorry. I was expecting that constructors are singletons like classes. No problem, I had the same expectation and was a little disappointed to see that it didn't work! I think maybe the whole Predicate approach is too much detailed? I think it's worth exluding with a predicate: for example this allows to test random chains with LimitTokenCountFilter(consumeAllTokens=true) (when consumeAllTokens=false, this filter is broken). I would exclude all broken construcors with the ALWAYS predicate in beforeClass() Sounds good, I updated the patch.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Adrien Grand
          http://svn.apache.org/viewvc?view=revision&revision=1430931

          LUCENE-4667: Change the broken components list from class-based to constructor-based.

          TestRandomChains now tests LimitTokenCountFilter and checks that offsets
          generated with TrimFilter and TypeTokenFilter are correct.

          Show
          Commit Tag Bot added a comment - [trunk commit] Adrien Grand http://svn.apache.org/viewvc?view=revision&revision=1430931 LUCENE-4667 : Change the broken components list from class-based to constructor-based. TestRandomChains now tests LimitTokenCountFilter and checks that offsets generated with TrimFilter and TypeTokenFilter are correct.
          Hide
          Robert Muir added a comment -

          We could also use this to stop some false fails from all the subclasses of FilteringTokenFilter (LengthFilter, TypeFilter, etc)
          that currently cause failures due to https://issues.apache.org/jira/browse/LUCENE-4065, when enablePositionIncrements=false

          Show
          Robert Muir added a comment - We could also use this to stop some false fails from all the subclasses of FilteringTokenFilter (LengthFilter, TypeFilter, etc) that currently cause failures due to https://issues.apache.org/jira/browse/LUCENE-4065 , when enablePositionIncrements=false
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Adrien Grand
          http://svn.apache.org/viewvc?view=revision&revision=1430934

          LUCENE-4667: Change the broken components list from class-based to constructor-based (merged from r1430931).

          TestRandomChains now tests LimitTokenCountFilter and checks that offsets
          generated with TrimFilter and TypeTokenFilter are correct.

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Adrien Grand http://svn.apache.org/viewvc?view=revision&revision=1430934 LUCENE-4667 : Change the broken components list from class-based to constructor-based (merged from r1430931). TestRandomChains now tests LimitTokenCountFilter and checks that offsets generated with TrimFilter and TypeTokenFilter are correct.
          Hide
          Uwe Schindler added a comment -

          Thanks Adrien!

          Show
          Uwe Schindler added a comment - Thanks Adrien!

            People

            • Assignee:
              Adrien Grand
              Reporter:
              Adrien Grand
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development