Conversation
|
Test cases pass but do not meet jacoco line coverage case (see lines 1194 and 1201 of Travis CI build). The phrases "this" and "string" come up a lot in your tests - I think changing some tests to use more varied strings should help it pass the jacoco check. Looks great otherwise! |
There was a problem hiding this comment.
Your implementation and most of your tests are fine, but I don't think we're supposed to modify the lines covered ratio in pom.xml. I left a comment on the pom file explaining it further.
I think if you change one or two tests to check that exceptions are thrown properly then your code should pass for the original 0.9 minimum lines covered ratio.
| @Test | ||
| public void testGeSubStrings2() { | ||
| fail("Not yet implemented"); | ||
| manipulatedstring.setString("This is another string"); |
There was a problem hiding this comment.
Change input string to a more unique statement (specifically "this" and "string")
Consider updating subsequent tests as well
There was a problem hiding this comment.
since I'm gonna change it I made it so that it's also checking on the throw exception for this
| } | ||
|
|
||
| String theString = getString(); | ||
| StringBuilder sb = new StringBuilder(theString); |
There was a problem hiding this comment.
Good idea using a StringBuilder! I hadn't thought of that in my implementation.
There was a problem hiding this comment.
But I forgot to add a exception when n is larger than the length of string lol
| return null; | ||
| } | ||
| public String restoreString(int[] indices){ | ||
| if(indices == null || this.pString == null) { |
There was a problem hiding this comment.
May also want to check for string and indices[] with length zero as a precaution
| @Test | ||
| public void testRemoveNthCharacter5() { | ||
| fail("Not yet implemented"); | ||
| manipulatedstring.setString("This message should be unreadable"); |
There was a problem hiding this comment.
I'd recommend adding a test that checks to see what happens when N is a value larger than the length of the string
pom.xml
Outdated
| <counter>LINE</counter> | ||
| <value>COVEREDRATIO</value> | ||
| <minimum>0.9</minimum> | ||
| <minimum>0.8</minimum> |
There was a problem hiding this comment.
Changing this value isn't a great idea because it tells Travis how much of your code needs to be checked by your unit tests to pass. It looks like your previous commit wasn't building because some of your tests weren't diverse enough to test all parts of your code.
Here's a site that explains it better: https://mkyong.com/maven/maven-jacoco-code-coverage-example/
There was a problem hiding this comment.
Thanks for letting me know I'm gonna change that before I push it again
carector
left a comment
carector
left a comment
There was a problem hiding this comment.
Looks good! Thanks for fixing the unit tests!
2 participants
No description provided.