SOLR-12555: refactor tests in package org.apache.solr.search #464
Conversation
barrotsteindev
changed the title
| //expected | ||
| } | ||
| assertFalse(equals); | ||
| expectThrows(AssertionError.class, "expected error, functions are not equal", |
There was a problem hiding this comment.
[0] Not suggesting you change it here, but....kindof weird that there's just not an assertFuncNotEquals
| } catch(AssertionFailedError e) { | ||
| assertTrue("queries were not equal, as expected", true); | ||
| } | ||
| expectThrows(AssertionFailedError.class, "queries were not equal, as expected", |
There was a problem hiding this comment.
[-1] I think this exception message here is backwards. This assertion fails if the queries were equal, but the message indicates that the problem is that they were !=. Using the message from the original fail() invocation would probably work better here.
| } catch(AssertionFailedError e) { | ||
| assertTrue("queries were not equal, as expected", true); | ||
| } | ||
| expectThrows(AssertionFailedError.class, "queries were not equal, as expected", |
There was a problem hiding this comment.
[-1] I think this exception message here is backwards. This assertion fails if the queries were equal, but the message implies that the problem is that they were !=. Using the message from the original fail() invocation would probably work better here ("queries should not have been equal")
| assertTrue("queries were not equal, as expected", true); | ||
| } | ||
|
|
||
| expectThrows(AssertionFailedError.class, "queries were not equal, as expected", |
There was a problem hiding this comment.
[-1] ditto re: wrong String message in expectThrows here
| fail("Cyclic alising not detected"); | ||
| } catch (SolrException e) { | ||
| assertTrue(e.getCause().getMessage().contains("Field aliases lead to a cycle")); | ||
| } catch (SolrException ex) { |
There was a problem hiding this comment.
[Q] Is there a reason that this example also couldn't be changed into an expectThrows?
There was a problem hiding this comment.
My bad,
I sort of missed that one.
Just pushed a new commit fixing it though.
|
|
||
| version = version2; | ||
| final long prevVersion = version2; |
There was a problem hiding this comment.
[0] I'm still somewhat leery of changing how the version variables are used here. I agree with what seems like your intent here - that final variables often make it much easier to reason about Java code. But with how flaky the tests are, I'd rather not introduce such changes here.
There was a problem hiding this comment.
The only problem is that if the variables are not final, they can not be used inside the lambda that is passed to expectThrows.
Should I just revert these changes?
There was a problem hiding this comment.
Oof, yeah good point, that would be a problem. No need to revert. I'll just need to pay particular attention to this class when testing things out.
barrotsteindev
changed the title
|
Thanks again for the work Bar. I've merged this to |
Tests are refactored to use expectThrows instead of try/catch.