[jira] [Updated] (SOLR-15629) replace SolrException.ignorePatterns with a a new test-framework Rule

["Chris M. Hostetter (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SOLR-15629?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Chris M. Hostetter updated SOLR-15629:
--------------------------------------
    Attachment: SOLR-15629.patch
      Assignee: Chris M. Hostetter
        Status: Open  (was: Open)

{quote}a "lambda wrapper" (similar to how {{expectException(...)}} works) would probably make more sense...
{quote}
I briefly toyed with this, but quickly realized it was actaully a big pain in the ass, because there's no easy way for a method like this to propogate any Throwable thrown by the wrapped lambda w/o just delcaring "throws Throwable" (not an issue for {{expectException(...)}} because by design it *wants* to catch exceptions and it *wants* to "fail" if a different type of exception is thrown)

I realized what makes a lot more sense is a {{LogInterceptor implements AutoClosable}} that sets up the log Filter in it's constructor/factory method, and removes the interception in it's {{close()}} method – that way you can use a "try-with-resources" to wrap the code you want to ignore exceptions in.

A Pro/Con of this approach is that you can call methods on the {{LogInterceptor}} to "inspect" the intercepted log messages inside the try block, w/o needing to wait until all of the logic in the block is done ... the "Con" part being that you *MUST* call at least some method on the {{LogInterceptor}} inside the try block, or the compiler warn/fails tha it's unused. (Which i think is actually a "Pro": If a test wants to expect/ignore some ERROR message logs, it should assert something about them)

The attached patch implements this idea – but I should note that I decided pretty quickly that because of how log4j (and it's APIs) work, just having each {{LogInterceptor}} add a {{Filter}} on the root logger ddin't seem like it would really cut it if we also wanted to support "inspection"; because if we wanted to have multiple {{LogInterceptor}} 's in use at the same time, each interceptor/Filter could potentially get in each others way. It seems cleaner to ask you what logger to "inspect" and use an Appender there with a Filter that only ACCEPTs the LogEvents you are interested in, while also adding a Filter to the ROOT logger to prevent those ERRORs from making it to the log file.

The patch works, and you can see from the tests what it might look like if we started using it – but the more i've been working on it, the more disatisfied i've become with the both the surface API and a llot of the implementation details.

I won't elaborate too much here in jira – it's pretty thoroughly spelled out in nocommit comments in the patch, where i also flesh out what i think would be better – for now i'll just point out:
 * I do plan to keep working on this
 * If i get hit by a bus and never finish this, please remember:
 ** a lot of complexity in the current patch (and in the suggestions in the nocommit comments) comes from wanting to implement something *BETTER* then the existing {{SolrTestCaseJ4.ignoreException}} functionality
 *** I'd really like us to be able write better tests that are more strict about if/when they expect an ERROR (or a WARN) to be logged
 *** ideally even start failing the build if any test causes solr to log an ERROR that isn't expected
 ** The original idea of just (temporarily) adding a Filter to the ROOT logger (that DENY's an ERROR log matching the Pattern or (sub)String) is still very viable and would be simple to implement as an alternative if someone is interested.

> replace SolrException.ignorePatterns with a a new test-framework Rule 
> ----------------------------------------------------------------------
>
>                 Key: SOLR-15629
>                 URL: https://issues.apache.org/jira/browse/SOLR-15629
>             Project: Solr
>          Issue Type: Improvement
>            Reporter: Chris M. Hostetter
>            Assignee: Chris M. Hostetter
>            Priority: Major
>         Attachments: SOLR-15629.patch
>
>
> We should deprecated & remove the following code:
>  * {{SolrException.log()}}
>  * {{SolrException.ignorePatterns}}
>  * {{SolrTestCaseJ4.ignoreException()}}
>  * {{SolrTestCaseJ4.unIgnoreException()}}
>  * {{SolrTestCaseJ4.resetExceptionIgnores()}}
> ...and replace with something along the lines of ... (from SOLR-15628) ...
> {quote}We should probably re-think the entire existence of {{SolrException.log()}} and the API design of {{SolrException.ignorePatterns}} – replacing all callers of {{SolrException.log()}} with {{logger.error()}} and use a new test-only log4j Filter/Appdener
> {quote}
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org