New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ISPN-7650 Refactoring two functional test assertions #5003
Conversation
assert false : "Should have thrown a UnsupportedOperationException"; | ||
} catch (UnsupportedOperationException uoe) { | ||
} | ||
col.add(newObj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you want to test adding to both keys and values - this will throw exception with keys and the values won't be tested.
IMO having Collection[] { keys, values }
is a strange pattern; I would rather use keys.add(newObj);
and values.add(newObj)
separately.
Note that instead of try-catch or @Test(expectedExceptions = ...)
you can also use Exceptions.expectException(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok 👍
|
||
assertOnAllCachesAndOwnership("k1", "value"); | ||
|
||
assert !nonOwner.getAdvancedCache().getComponentRegistry().getComponent(DistributionManager.class).getCacheTopology().isWriteOwner("k1"); | ||
assertFalse(nonOwner.getAdvancedCache().getComponentRegistry().getComponent(DistributionManager.class).getCacheTopology().isWriteOwner("k1")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a helper method TestingUtil.extractComponent
@rvansa I modified both tests with your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can any of the devs integrate it?
Integrated, thanks @karesti |
Oops, I have noticed only now that it uses TestNG assertions; eventually all codebase should use only the AssertJUnit ones (despite the annotations are TestNG). My fault, I should have pointed it out during the review earlier; it's not an issue, but @karesti in the future please use AssertJUnit. |
Yes, indeed I realised it just after too. No pb, I will refactor them on my branches to import the good ones if nobody does it before |
https://issues.jboss.org/browse/ISPN-7650