-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
review: test: add test for SubstituionInsertAllNestedTypes #3998
review: test: add test for SubstituionInsertAllNestedTypes #3998
Conversation
Link to #3967 |
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.
Looks good, I just have some minor comments on this one.
Co-authored-by: Simon Larsén <[email protected]>
Co-authored-by: Simon Larsén <[email protected]>
@slarse I would be glad if you may have again have a look at the PR : ) |
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.
Looks good to me! Just had a minor note on the new assertion's layout. But I think this is fine to merge, if you want to fix anything up at a later time you can just come back here.
|
||
// assert | ||
assertEquals(1, targetType.getNestedTypes().size()); | ||
assertEquals("nestedClass", targetType.getNestedType("nestedClass").getSimpleName()); |
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.
The new assertion strengthens the test case in the sense that it verifies that there's a nested type with name nestedClass
, and that's what was requested. So I think this is fine now.
However, I just wanted to note that the layout of the assertion makes it look like a test for CtType.getNestedType(String)
. To be clear, it looks like the assertion verifies that getting a nested type by a given name returns a type with that name. It would be sufficient here to just verify that the returned type is non-null, due to the functionality of getNestedType
.
assertEquals("nestedClass", targetType.getNestedType("nestedClass").getSimpleName()); | |
assertNotNull(targetType.getNestedType("nestedClass")); |
Personally, I would probably have written both assertions like this instead, so as not to have to query targetType
multiple times.
List<CtType<?>> nestedTypes = targetType.getNestedTypes();
assertThat(nestedTypes.size(), equalTo(1));
assertThat(nestedTypes.get(0), equalTo("nestedClass"));
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.
Thanks for the suggestions, so I can add this in the PR that I will raise for another new test?
Co-authored-by: Simon Larsén <[email protected]>
#1818
Hi, I have added a new test for
insertAllNestedFilelds
method of theSubstitution
class, this test improves code coverage, mutation converge and test strength all from 0% to 100%.Here's the report before improvement.
The exception added in this test, is able to kill the mutation but, probably isn't the strongest assertion, I wan't able to make it stronger.