Lucene - Core
  1. Lucene - Core
  2. LUCENE-4630

add a system property to allow testing of suspicious stuff

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 4.9, 5.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      there are times when people want to add assumptions in test to prevent confusing/false failures in certain situations (eg: known bugs in JVM X, known incompatibilities between lucene feature Z and filesystem Y, etc...)

      By default we want these situations to be "skiped" in tests with clear messages so that it's clear to end users trying out releases that these tests can't be run for specific sitautions.

      But at the same time we need a way for developers to be able to try running these tests anyway so we know if/when the underliyng problem is resolved.

      i propose we add a "tests.suspicious.shit" system property, which defaults to "false" in the javacode, but can be set at runtime to "true"

      assumptions about things like incompatibilities with OSs, JVM vendors, JVM versions, filesystems, etc.. can all be dependent on this system propery.

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          I am fine with such a system property and also some code to warn user of several incompatible combinations, but I want to be able to run tests to find the problems behind the issue.

          In my opinion, we should really warn users also on Solr startup, if they have jRockit (this JVM only works with Lucene if you pass -XnoOpt) or J9 (fails with Lucene 4.0+), so they don't corrumpt their index. Please note: Policeman Jenkins (before it was shot by some Generics Drug Dealer) was running JRockit with this JVM option.

          Show
          Uwe Schindler added a comment - I am fine with such a system property and also some code to warn user of several incompatible combinations, but I want to be able to run tests to find the problems behind the issue. In my opinion, we should really warn users also on Solr startup, if they have jRockit (this JVM only works with Lucene if you pass -XnoOpt) or J9 (fails with Lucene 4.0+), so they don't corrumpt their index. Please note: Policeman Jenkins (before it was shot by some Generics Drug Dealer) was running JRockit with this JVM option.
          Hide
          Dawid Weiss added a comment -

          Why does it need to be a system property, Hoss? The test group annotations can be enabled/disabled via system properties and they also do display messages on assumption-ignored tests – wouldn't this be enough to cover your use case?

          @SuspiciousJ9Shit
          @SuspiciousJRockitShit
          

          The only problem I see is that these need to be provided statically – if you need to detect them at runtime then I'd either need to change the code of the runner or we'd need to switch to assumptions inside a rule, for example.

          Show
          Dawid Weiss added a comment - Why does it need to be a system property, Hoss? The test group annotations can be enabled/disabled via system properties and they also do display messages on assumption-ignored tests – wouldn't this be enough to cover your use case? @SuspiciousJ9Shit @SuspiciousJRockitShit The only problem I see is that these need to be provided statically – if you need to detect them at runtime then I'd either need to change the code of the runner or we'd need to switch to assumptions inside a rule, for example.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Uwe Schindler
          http://svn.apache.org/viewvc?view=revision&revision=1421818

          Revert the revert of the revert. I hope Robert Muir will followup and we can agree that this patch is not really a good idea and we should discuss on LUCENE-4630 what to do. I know that Mike is already beasting J9, so we would need an option to disable this AssumptionFailedEx. Thanks in advance!

          Show
          Commit Tag Bot added a comment - [trunk commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1421818 Revert the revert of the revert. I hope Robert Muir will followup and we can agree that this patch is not really a good idea and we should discuss on LUCENE-4630 what to do. I know that Mike is already beasting J9, so we would need an option to disable this AssumptionFailedEx. Thanks in advance!
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Uwe Schindler
          http://svn.apache.org/viewvc?view=revision&revision=1421819

          Merged revision(s) 1421818 from lucene/dev/trunk:
          Revert the revert of the revert. I hope Robert Muir will followup and we can agree that this patch is not really a good idea and we should discuss on LUCENE-4630 what to do. I know that Mike is already beasting J9, so we would need an option to disable this AssumptionFailedEx. Thanks in advance!

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Uwe Schindler http://svn.apache.org/viewvc?view=revision&revision=1421819 Merged revision(s) 1421818 from lucene/dev/trunk: Revert the revert of the revert. I hope Robert Muir will followup and we can agree that this patch is not really a good idea and we should discuss on LUCENE-4630 what to do. I know that Mike is already beasting J9, so we would need an option to disable this AssumptionFailedEx. Thanks in advance!
          Hide
          Hoss Man added a comment -

          In my opinion, we should really warn users also on Solr startup, if they have...

          This is a great idea – i've spun off LUCENE-4631 to track that since it's kind of broader then if/how to allow people to run tests even under suspicious circumstances (but whatever we add there can probably be leveraged here)

          Why does it need to be a system property, Hoss?

          No reason ... it was just the first thing i thought of that seemed really generic. Anyplace we might normally write...

          boolean someBoolean = ...something interesting...;
          if (someBoolean) {
            throw new AssumptionViolatedException("...why your system is suspicious...")
          }
          

          ...could be replaced with...

          boolean someBoolean = ...something interesting...;
          if (someBoolean || Boolean.getBoolean("tests.suspicious.shit")) {
            throw new AssumptionViolatedException("...why your system is suspicious...")
          }
          

          ...of the top of my head, i wasn't sure if an annotation would be as easy to use (particularly when you might mix and match with other test groups)

          @SuspiciousJ9Shit

          My suggestion was to try and keep it reaallly generic ... so that with one "option" active you say "i'm a developer who is asking for trouble, try everything even if you think it's not valid on my system. Making people know that their particular os/jvm/filesystem/jvm-opt-combos are suspicious therefore they need to explicitly ask for test group X and test group Y test group Z seems like it would make it overly hard for people to try everything.

          Perhaps the ideal case would be specific annotations like you describe, which could be used as test groups for people who want to go out of their way to test specific suspicious stuff (ie: "there is a new J9 JVM, does it still have these problems?") but then have a feature in the runner that by default those groups are skipped with a clear AssumptionViolatedException("...what is suspicious about your setup ...") but if you set "-Dtest.suspicious.shit=true" then instead the runner will run those tests anyway, but wrap any failures/exceptions it gets in another "FailureUnderSuspiciousCircumstances" exception whose getMessage() would contain info about what assumption would have normally prevented that test from if you hadn't gone out of your way to run it.

          what do you think?

          Show
          Hoss Man added a comment - In my opinion, we should really warn users also on Solr startup, if they have... This is a great idea – i've spun off LUCENE-4631 to track that since it's kind of broader then if/how to allow people to run tests even under suspicious circumstances (but whatever we add there can probably be leveraged here) Why does it need to be a system property, Hoss? No reason ... it was just the first thing i thought of that seemed really generic. Anyplace we might normally write... boolean someBoolean = ...something interesting...; if (someBoolean) { throw new AssumptionViolatedException( "...why your system is suspicious..." ) } ...could be replaced with... boolean someBoolean = ...something interesting...; if (someBoolean || Boolean .getBoolean( "tests.suspicious.shit" )) { throw new AssumptionViolatedException( "...why your system is suspicious..." ) } ...of the top of my head, i wasn't sure if an annotation would be as easy to use (particularly when you might mix and match with other test groups) @SuspiciousJ9Shit My suggestion was to try and keep it reaallly generic ... so that with one "option" active you say "i'm a developer who is asking for trouble, try everything even if you think it's not valid on my system. Making people know that their particular os/jvm/filesystem/jvm-opt-combos are suspicious therefore they need to explicitly ask for test group X and test group Y test group Z seems like it would make it overly hard for people to try everything. Perhaps the ideal case would be specific annotations like you describe, which could be used as test groups for people who want to go out of their way to test specific suspicious stuff (ie: "there is a new J9 JVM, does it still have these problems?") but then have a feature in the runner that by default those groups are skipped with a clear AssumptionViolatedException("...what is suspicious about your setup ...") but if you set "-Dtest.suspicious.shit=true" then instead the runner will run those tests anyway, but wrap any failures/exceptions it gets in another "FailureUnderSuspiciousCircumstances" exception whose getMessage() would contain info about what assumption would have normally prevented that test from if you hadn't gone out of your way to run it. what do you think?
          Hide
          Dawid Weiss added a comment -

          but if you set "-Dtest.suspicious.shit=true" then instead the runner will run those tests anyway,

          This you can do already; test groups can be turned on and off by overriding their assigned system property, no problem with that.

          but wrap any failures/exceptions it gets in another "FailureUnderSuspiciousCircumstances" exception whose getMessage() would contain info about what assumption would have normally prevented that test from if you hadn't gone out of your way to run it.

          Even this description makes me feel dizzy... I get your idea but I don't know how to implement it in a sensible way. It could be a rule that would intercept failures, check for groups annotations and then rethrow... but I honestly don't think many people would find it useful (or understand the principle under which it operates).

          Dawid

          Show
          Dawid Weiss added a comment - but if you set "-Dtest.suspicious.shit=true" then instead the runner will run those tests anyway, This you can do already; test groups can be turned on and off by overriding their assigned system property, no problem with that. but wrap any failures/exceptions it gets in another "FailureUnderSuspiciousCircumstances" exception whose getMessage() would contain info about what assumption would have normally prevented that test from if you hadn't gone out of your way to run it. Even this description makes me feel dizzy... I get your idea but I don't know how to implement it in a sensible way. It could be a rule that would intercept failures, check for groups annotations and then rethrow... but I honestly don't think many people would find it useful (or understand the principle under which it operates). Dawid
          Hide
          Steve Rowe added a comment -

          Bulk move 4.4 issues to 4.5 and 5.0

          Show
          Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
          Hide
          Uwe Schindler added a comment -

          Move issue to Lucene 4.9.

          Show
          Uwe Schindler added a comment - Move issue to Lucene 4.9.

            People

            • Assignee:
              Unassigned
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development