[ https://issues.apache.org/jira/browse/SOLR-12555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16693658#comment-16693658 ]
Bar Rotstein edited comment on SOLR-12555 at 11/20/18 7:01 PM: --------------------------------------------------------------- You've done this very quickly I feel like we are getting closer to closing this ticket. Great patch, except for two really minor inputs: 1. I'd change SolrResourceLoader#testWrongEncoding:142 from {code:java} SolrResourceLoader loader = new SolrResourceLoader(TEST_PATH().resolve("collection1")); // ensure we get our exception SolrException thrown = expectThrows(SolrException.class, () -> loader.getLines(wrongEncoding)); assertTrue(thrown.getCause() instanceof CharacterCodingException); loader.close();{code} to {code:java} try(SolrResourceLoader loader = new SolrResourceLoader(TEST_PATH().resolve("collection1"))) { // ensure we get our exception SolrException thrown = expectThrows(SolrException.class, () -> loader.getLines(wrongEncoding)); assertTrue(thrown.getCause() instanceof CharacterCodingException); }{code} 2. Missing assertThat in TestCoreDiscovery#testSolrHomeNotReadable:530 {code:java} Exception thrown = expectThrows(Exception.class, () -> { CoreContainer cc = null; try { cc = init(); } finally { if (cc != null) cc.shutdown(); } }); assertThat(thrown.getMessage(), containsString("Error reading core root directory"));{code} Other than these two minor differences, I edited out a few blank lines. Hopefully we can keep up the pace. I'll upload a patch file with these changes. was (Author: brot): You've done this very quickly I feel like we are getting closer to closing this ticket. Great patch, except for two really minor inputs I have. 1. I'd change SolrResourceLoader#testWrongEncoding:142 from {code:java} SolrResourceLoader loader = new SolrResourceLoader(TEST_PATH().resolve("collection1")); // ensure we get our exception SolrException thrown = expectThrows(SolrException.class, () -> loader.getLines(wrongEncoding)); assertTrue(thrown.getCause() instanceof CharacterCodingException); loader.close();{code} to {code:java} try(SolrResourceLoader loader = new SolrResourceLoader(TEST_PATH().resolve("collection1"))) { // ensure we get our exception SolrException thrown = expectThrows(SolrException.class, () -> loader.getLines(wrongEncoding)); assertTrue(thrown.getCause() instanceof CharacterCodingException); }{code} 2. Missing assertThat in TestCoreDiscovery#testSolrHomeNotReadable:530 {code:java} Exception thrown = expectThrows(Exception.class, () -> { CoreContainer cc = null; try { cc = init(); } finally { if (cc != null) cc.shutdown(); } }); assertThat(thrown.getMessage(), containsString("Error reading core root directory"));{code} Other than these two minor differences, I edited out a few blank lines. Hopefully we can keep up the pace. I'll upload a patch file with these changes. > Replace try-fail-catch test patterns > ------------------------------------ > > Key: SOLR-12555 > URL: https://issues.apache.org/jira/browse/SOLR-12555 > Project: Solr > Issue Type: Test > Security Level: Public(Default Security Level. Issues are Public) > Components: Tests > Affects Versions: master (8.0) > Reporter: Jason Gerlowski > Assignee: Jason Gerlowski > Priority: Trivial > Attachments: SOLR-12555-sorted-by-package.txt, SOLR-12555.patch, > SOLR-12555.patch, SOLR-12555.txt > > Time Spent: 4h 20m > Remaining Estimate: 0h > > I recently added some test code through SOLR-12427 which used the following > test anti-pattern: > {code} > try { > actionExpectedToThrowException(); > fail("I expected this to throw an exception, but it didn't"); > catch (Exception e) { > assertOnThrownException(e); > } > {code} > Hoss (rightfully) objected that this should instead be written using the > formulation below, which is clearer and more concise. > {code} > SolrException e = expectThrows(() -> {...}); > {code} > We should remove many of these older formulations where it makes sense. Many > of them were written before {{expectThrows}} was introduced, and having the > old style assertions around makes it easier for them to continue creeping in. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org