Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8184

Negative tests for JDBC Connection String

    Details

    • Type: Test
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: None
    • Fix Version/s: 6.1, master (7.0)
    • Component/s: None
    • Labels:
      None
    • Environment:

      Trunk

    • Flags:
      Patch

      Description

      Ticket to track negative tests for JDBC connection string SOLR-7986

      1. SOLR-8184.patch
        4 kB
        Kevin Risden
      2. SOLR-8184.patch
        2 kB
        Jason Gerlowski
      3. SOLR-8184.patch
        3 kB
        Jason Gerlowski
      4. SOLR-8184.patch
        3 kB
        Susheel Kumar

        Issue Links

          Activity

          Hide
          susheel2777@gmail.com Susheel Kumar added a comment -

          Negative tests for JDBC Connection String

          Show
          susheel2777@gmail.com Susheel Kumar added a comment - Negative tests for JDBC Connection String
          Hide
          susheel2777@gmail.com Susheel Kumar added a comment -

          Some observations:

          The test "testConnectionStringWithMissingZKHost" throws SolrException / TimeOutException than SQLException and similarly the test "testConnectionStringWithWrongCollection" goes thru various retries before it fails. Is that right behavior?

          Show
          susheel2777@gmail.com Susheel Kumar added a comment - Some observations: The test "testConnectionStringWithMissingZKHost" throws SolrException / TimeOutException than SQLException and similarly the test "testConnectionStringWithWrongCollection" goes thru various retries before it fails. Is that right behavior?
          Hide
          risdenk Kevin Risden added a comment -

          Susheel Kumar Are the tests that were added here included in SOLR-8179?

          The way the tests that were added w/ your patch would mean that a whole new Solr cluster would be stood up for each test. In SOLR-8179 I added a new class specifically for testing the driver that doesn't require a Solr cluster to be up and running.

          Show
          risdenk Kevin Risden added a comment - Susheel Kumar Are the tests that were added here included in SOLR-8179 ? The way the tests that were added w/ your patch would mean that a whole new Solr cluster would be stood up for each test. In SOLR-8179 I added a new class specifically for testing the driver that doesn't require a Solr cluster to be up and running.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          I've updated this patch to work on the latest trunk.

          I had to make a few changes to allow the tests to pass:
          1.) testConnectionStringWithWrongCollection expected SolrException in the original patch, however it turns out that on the latest trunk, it's SQLException that's actually thrown. I changed the exception-catching to reflect this.
          2.) testConnectionStringWithWrongCollection's use of ExpectedException prevented it from closing the db connection. I had to tweak this so that it could be closed.

          Still looking into whether the tests would run more efficiently under a different base-class. Though I did baseline the performance of the tests in JdbcTest. Without the additions in this patch, JdbcTest takes about 2min 15sec to run on my machine. With this patch, it takes about 3 mins to run. The ~45sec delta makes me suspect that the cases are being run inefficiently, but I haven't verified that yet.

          Show
          gerlowskija Jason Gerlowski added a comment - I've updated this patch to work on the latest trunk. I had to make a few changes to allow the tests to pass: 1.) testConnectionStringWithWrongCollection expected SolrException in the original patch, however it turns out that on the latest trunk, it's SQLException that's actually thrown. I changed the exception-catching to reflect this. 2.) testConnectionStringWithWrongCollection 's use of ExpectedException prevented it from closing the db connection. I had to tweak this so that it could be closed. Still looking into whether the tests would run more efficiently under a different base-class. Though I did baseline the performance of the tests in JdbcTest. Without the additions in this patch, JdbcTest takes about 2min 15sec to run on my machine. With this patch, it takes about 3 mins to run. The ~45sec delta makes me suspect that the cases are being run inefficiently, but I haven't verified that yet.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          To follow up on my prior comment, it turns out that the test file Kevin referred to above (JdbcDriverTest) does involve much less overhead (it runs in ~1sec on my machine.). Since the overheard on these tests is much lower, I second Kevin's recommendation that we move everything we can into JdbcDriverTest .

          There are really 3 tests in question here: testConnectionStringWithMissingZkHost, testConnectionStringJumbled, and testConnectionStringWithWrongCollection. The first of these is already covered by JdbcDriverTest.testNullZkConnectionString. The second doesn't have a JdbcDriverTest analog, but could easily be moved over to JdbcDriverTest. The third test case would be more difficult to move, as it involves checking an error arises after connecting, creating a statement, etc. There doesn't look like there's an easy way to port this over.

          My suggestion/vote would be:
          1.) Totally drop the first test (since it already has coverage in JdbcDriverTest),
          2.) Move the second test to JdbcDriverTest,
          3.) Keep the third test in JdbcTest, but move it into the doTest() method. This allows it to avoid incurring the unnecessary overheard, as Kevin mentioned above.

          Anyone have thoughts/suggestions/counterarguments? I'm happy to make these modifications myself if others find them reasonable, but I don't want to step on any toes. Susheel Kumar pushed the first revision of this up, so I'll hold off for a few days to see if he has any thoughts.

          Otherwise (assuming people are ok with my proposed plan), I'll go ahead with this.

          Show
          gerlowskija Jason Gerlowski added a comment - To follow up on my prior comment, it turns out that the test file Kevin referred to above (JdbcDriverTest) does involve much less overhead (it runs in ~1sec on my machine.). Since the overheard on these tests is much lower, I second Kevin's recommendation that we move everything we can into JdbcDriverTest . There are really 3 tests in question here: testConnectionStringWithMissingZkHost, testConnectionStringJumbled, and testConnectionStringWithWrongCollection. The first of these is already covered by JdbcDriverTest.testNullZkConnectionString. The second doesn't have a JdbcDriverTest analog, but could easily be moved over to JdbcDriverTest. The third test case would be more difficult to move, as it involves checking an error arises after connecting, creating a statement, etc. There doesn't look like there's an easy way to port this over. My suggestion/vote would be: 1.) Totally drop the first test (since it already has coverage in JdbcDriverTest), 2.) Move the second test to JdbcDriverTest, 3.) Keep the third test in JdbcTest, but move it into the doTest() method. This allows it to avoid incurring the unnecessary overheard, as Kevin mentioned above. Anyone have thoughts/suggestions/counterarguments? I'm happy to make these modifications myself if others find them reasonable, but I don't want to step on any toes. Susheel Kumar pushed the first revision of this up, so I'll hold off for a few days to see if he has any thoughts. Otherwise (assuming people are ok with my proposed plan), I'll go ahead with this.
          Hide
          risdenk Kevin Risden added a comment -

          1.) Totally drop the first test (since it already has coverage in JdbcDriverTest),
          2.) Move the second test to JdbcDriverTest,
          3.) Keep the third test in JdbcTest, but move it into the doTest() method. This allows it to avoid incurring the unnecessary overheard, as Kevin mentioned above.

          sounds reasonable to me.

          Show
          risdenk Kevin Risden added a comment - 1.) Totally drop the first test (since it already has coverage in JdbcDriverTest), 2.) Move the second test to JdbcDriverTest, 3.) Keep the third test in JdbcTest, but move it into the doTest() method. This allows it to avoid incurring the unnecessary overheard, as Kevin mentioned above. sounds reasonable to me.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          Updated patch takes the approach mentioned in the comments above (drop duplicate test, move tests into JdbcDriverTest where possible, fold remaining test into same test case of JdbcTest).

          Should be ready to go as far as I know, unless anyone knows of error cases they'd like covered by this story?

          Show
          gerlowskija Jason Gerlowski added a comment - Updated patch takes the approach mentioned in the comments above (drop duplicate test, move tests into JdbcDriverTest where possible, fold remaining test into same test case of JdbcTest). Should be ready to go as far as I know, unless anyone knows of error cases they'd like covered by this story?
          Hide
          risdenk Kevin Risden added a comment - - edited

          What is this testing for exactly? It looks like the connection string will be valid? A SQLException will be thrown because it can't connect to the cluster though.

          +  @Test(expected = SQLException.class)
          +  public void testConnectionStringJumbled() throws Exception {
          +    final String sampleZkHost="zoo1:9983/foo";
          +    DriverManager.getConnection("solr:jdbc://" + sampleZkHost + "?collection=collection1", new Properties());
          +  }
          
          Show
          risdenk Kevin Risden added a comment - - edited What is this testing for exactly? It looks like the connection string will be valid? A SQLException will be thrown because it can't connect to the cluster though. + @Test(expected = SQLException.class) + public void testConnectionStringJumbled() throws Exception { + final String sampleZkHost= "zoo1:9983/foo" ; + DriverManager.getConnection( "solr:jdbc: //" + sampleZkHost + "?collection=collection1" , new Properties()); + }
          Hide
          risdenk Kevin Risden added a comment -

          nvm I see that jdbc and solr are switched now.

          Show
          risdenk Kevin Risden added a comment - nvm I see that jdbc and solr are switched now.
          Hide
          gerlowskija Jason Gerlowski added a comment -

          My understanding is that a legit connection string starts with: jdbc://solr. This test has the position of "solr" and "jdbc" swapped (i.e. solr:jdbc://). So this connection string will cause an exception, even before the ZK host is parsed out and a connection is attempted.

          If you think this isn't clear enough, I'm happy to rename the method name (I kept the name from the initial patch). Or maybe assert on the exception message to make it clear that this isn't zk-related.

          Show
          gerlowskija Jason Gerlowski added a comment - My understanding is that a legit connection string starts with: jdbc://solr . This test has the position of "solr" and "jdbc" swapped (i.e. solr:jdbc:// ). So this connection string will cause an exception, even before the ZK host is parsed out and a connection is attempted. If you think this isn't clear enough, I'm happy to rename the method name (I kept the name from the initial patch). Or maybe assert on the exception message to make it clear that this isn't zk-related.
          Hide
          risdenk Kevin Risden added a comment -

          Yea whats really happening is that the DriverManager can't actually find a driver that supports that url string. The method that is getting tested is Driver#acceptsURL.

          java.sql.SQLException: No suitable driver found for solr:jdbc://zoo1:9983/foo?collection=collection1
          
          Show
          risdenk Kevin Risden added a comment - Yea whats really happening is that the DriverManager can't actually find a driver that supports that url string. The method that is getting tested is Driver#acceptsURL. java.sql.SQLException: No suitable driver found for solr:jdbc: //zoo1:9983/foo?collection=collection1
          Hide
          risdenk Kevin Risden added a comment -

          Attaching patch updated to master. Running precommit now.

          Show
          risdenk Kevin Risden added a comment - Attaching patch updated to master. Running precommit now.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 732e7e80c6fcb3d8ec2ecee3908dde88009f82d8 in lucene-solr's branch refs/heads/master from Kevin Risden
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=732e7e8 ]

          SOLR-8184: Negative tests for JDBC Connection String

          Show
          jira-bot ASF subversion and git services added a comment - Commit 732e7e80c6fcb3d8ec2ecee3908dde88009f82d8 in lucene-solr's branch refs/heads/master from Kevin Risden [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=732e7e8 ] SOLR-8184 : Negative tests for JDBC Connection String
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 219ddbb998cf0e011c087e7a0629908f40dd0d5d in lucene-solr's branch refs/heads/branch_6x from Kevin Risden
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=219ddbb ]

          SOLR-8184: Negative tests for JDBC Connection String

          Show
          jira-bot ASF subversion and git services added a comment - Commit 219ddbb998cf0e011c087e7a0629908f40dd0d5d in lucene-solr's branch refs/heads/branch_6x from Kevin Risden [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=219ddbb ] SOLR-8184 : Negative tests for JDBC Connection String
          Hide
          risdenk Kevin Risden added a comment -

          Thanks Susheel Kumar and Jason Gerlowski. Sorry took so long to get back to this.

          Show
          risdenk Kevin Risden added a comment - Thanks Susheel Kumar and Jason Gerlowski . Sorry took so long to get back to this.
          Hide
          hossman Hoss Man added a comment -

          Manually correcting fixVersion per Step #S5 of LUCENE-7271

          Show
          hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271

            People

            • Assignee:
              risdenk Kevin Risden
              Reporter:
              susheel2777@gmail.com Susheel Kumar
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development