[
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: [email protected]
For additional commands, e-mail: [email protected]
