[jira] [Comment Edited] (SOLR-13664) SolrTestCaseJ4.deleteCore() does not delete/clean dataDir

["Hoss Man (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SOLR-13664?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16897525#comment-16897525 ] 

Hoss Man edited comment on SOLR-13664 at 7/31/19 9:03 PM:
----------------------------------------------------------

Testing of the last patch uncovered 3 classes of problems in our existing tests:

# tests trying to call {{FileUtils.deleteDirectory(initCoreDataDir);}} after calling {{deleteCore()}} (specifically to work around this bug!) that now get NPE
#* Example: TestRecovery
# tests that need to be able to write files to {{initCoreDataDir}} *before* calling {{initCore()}} _and_ call {{initCore()}} + {{deleteCore()}} in the intividual test method life cycle
#* the patch already ensures {{initCoreDataDir}} is created before the subclass is initialized, so tests that just used a single {{initCore()}} call for all test methods would be fine -- it's only tests that also {{deleteCore()}} in {{@After}} methods that are problems
#* Example: QueryElevationComponentTest, SolrCoreCheckLockOnStartupTest
# one test that doesn't even use {{initCore()}} -- it builds it's own TestHarness/CoreContainer using {{initCoreDataDir}} directly -- but like #2, calls {{deleteCore()}} in {{@After}} methods (to leverate the common cleanup of the TestHarness)
#* Example: SolrMetricsIntegrationTest


Based on these classes of problems, I think the best way forward is to update the existing patch to:

* make the {{initAndGetDataDir()}} private helper method I introduced in the last patch public and change it to return the {{File}} (not String) and beef up it's javadocs
* deprecate {{initCoreDataDir}} and change all existing direct uses of it in our tests to use the helper method

This makes fixing the existing test problems trivial: just replace all uses of {{initCoreDataDir}} to {{initAndGetDataDir()}} ... any logic attempting to seed/inspect the dataDir prior to {{initCore()}} will initialize the directory that will be used by the next subsequent {{initCore()}} call.

----

I've attached an updated patch with this changes,  but! ... for completeness, I think it's important to also consider about how this will impact any existing third-party tests downstream users may have written that subclass {{SolrTestCaseJ4}:

* if they don't refer to {{initCoreDataDir}} directly, then like the existing patch the only change in behavior they should notice is that if their tests call {{deleteCore()}} any subsequent {{initCore()}} calls won't be polluted with the old data.
* if they do refer to {{initCoreDataDir}} directly in tests, then that usage _may_ continue to work as is if the usage is only "between" calls to {{initCore()}} and {{deleteCore()}} (ie: to inspect the data dir)
* if they attempt to use {{initCoreDataDir}} _after_ calling {{deleteCore()}} (either directly, or indirectly by referencing it before a call to {{initCore()}} in a test lifecycle that involves multiple {{initCore()}} + {{deleteCore()}} pairs) then they will start getting NPEs and will need to change their test to use {{initAndGetDataDir()}} directly.

I think the tradeoff of fixing this bug vs the impact on end users is worth making this change: right now the bug can silently affect users w/weird results, but any tests that are impacted adversly by this change will trigger loud NPEs and have an easy fix we can mention in the ugprade notes.




was (Author: hossman):

Testing of the last patch uncovered 3 classes of problems in our existing tests:

# tests trying to call {{FileUtils.deleteDirectory(initCoreDataDir);}} after calling {{deleteCore()}} (specifically to work around this bug!) that now get NPE
#* Example: TestRecovery
# tests that need to be able to write files to {{initCoreDataDir}} *before* calling {{initCore()}} _and_ call {{initCore()}} + {{deleteCore()}} in the intividual test method life cycle
#* the patch already ensures {{initCoreDataDir}} is created before the subclass is initialized, so tests that just used a single {{initCore()}} call for all test methods would be fine -- it's only tests that also {{deleteCore()}} in {{@After}} methods that are problems
#* Example: QueryElevationComponentTest, SolrCoreCheckLockOnStartupTest
* one test that doesn't even use {{initCore()}} -- it builds it's own TestHarness/CoreContainer using {{initCoreDataDir}} directly -- but like #2, calls {{deleteCore()}} in {{@After}} methods (to leverate the common cleanup of the TestHarness)
** Example: SolrMetricsIntegrationTest


Based on these classes of problems, I think the best way forward is to update the existing patch to:

* make the {{initAndGetDataDir()}} private helper method I introduced in the last patch public and change it to return the {{File}} (not String) and beef up it's javadocs
* deprecate {{initCoreDataDir}} and change all existing direct uses of it in our tests to use the helper method

This makes fixing the existing test problems trivial: just replace all uses of {{initCoreDataDir}} to {{initAndGetDataDir()}} ... any logic attempting to seed/inspect the dataDir prior to {{initCore()}} will initialize the directory that will be used by the next subsequent {{initCore()}} call.

----

I've attached an updated patch with this changes,  but! ... for completeness, I think it's important to also consider about how this will impact any existing third-party tests downstream users may have written that subclass {{SolrTestCaseJ4}:

* if they don't refer to {{initCoreDataDir}} directly, then like the existing patch the only change in behavior they should notice is that if their tests call {{deleteCore()}} any subsequent {{initCore()}} calls won't be polluted with the old data.
* if they do refer to {{initCoreDataDir}} directly in tests, then that usage _may_ continue to work as is if the usage is only "between" calls to {{initCore()}} and {{deleteCore()}} (ie: to inspect the data dir)
* if they attempt to use {{initCoreDataDir}} _after_ calling {{deleteCore()}} (either directly, or indirectly by referencing it before a call to {{initCore()}} in a test lifecycle that involves multiple {{initCore()}} + {{deleteCore()}} pairs) then they will start getting NPEs and will need to change their test to use {{initAndGetDataDir()}} directly.

I think the tradeoff of fixing this bug vs the impact on end users is worth making this change: right now the bug can silently affect users w/weird results, but any tests that are impacted adversly by this change will trigger loud NPEs and have an easy fix we can mention in the ugprade notes.



>  SolrTestCaseJ4.deleteCore() does not delete/clean dataDir
> ----------------------------------------------------------
>
>                 Key: SOLR-13664
>                 URL: https://issues.apache.org/jira/browse/SOLR-13664
>             Project: Solr
>          Issue Type: Bug
>      Security Level: Public(Default Security Level. Issues are Public) 
>            Reporter: Hoss Man
>            Assignee: Hoss Man
>            Priority: Major
>         Attachments: SOLR-13664.patch, SOLR-13664.patch, SOLR-13664.patch
>
>
> In spite of what it's javadocs say, {{SolrTestCaseJ4.deleteCore()}} does nothing to delete the dataDir used by the TestHarness
> The git history is a bit murky, so i'm not entirely certain when this stoped working, but I suspect it happened as part of the overall cleanup regarding test temp dirs and the use of {{LuceneTestCase.createTempDir(...) -> TestRuleTemporaryFilesCleanup}}
> While this is not problematic in many test classes, where a single {{initCore(...) is called in a {{@BeforeClass}} and the test then re-uses that SolrCore for all test methods and relies on {{@AfterClass SolrTestCaseJ4.teardownTestCases()}} to call {{deleteCore()}}, it's problematic in test classes where {{deleteCore()}} is explicitly called in an {{@After}} method to ensure a unique core (w/unique dataDir) is used for each test method.
> (there are currently about 61 tests that call {{deleteCore()}} directly)



--
This message was sent by Atlassian JIRA
(v7.6.14#76016)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org