Initial PageLoader Test ClassInitial test for the PageLoader class that covers null and empty checks #8
Conversation
for factory methods and constructors.
webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java
Show resolved
Hide resolved
| @SuppressWarnings("unused") | ||
| public class PageLoaderTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Should this be @Test(expected = NullPointerException.class)?
There was a problem hiding this comment.
@kpm1985 do you know if this is still an issue?
| public void testDeleteFailsWithNullUrl() { | ||
| try { | ||
| PageLoader loader = PageLoader.deletePage(null); | ||
| } catch (MalformedURLException e) { |
There was a problem hiding this comment.
Can this catch be dropped?
There was a problem hiding this comment.
I don't think so because of this method signature:
There was a problem hiding this comment.
If the exception is not expected you can make the test throw exception. If it is actually thrown then the test will fail.
There was a problem hiding this comment.
Not sure if this is correct but did a simple refactor where it catches Exception e and test if its an instance of a NPE and if yes throws NPE.
There was a problem hiding this comment.
I was thinking of the following.
@Test(expected = NullPointerException.class)
public void testDeletePageThrowsNPEifNullURL() throws MalformedURLException {
PageLoader.deletePage((URL)null);
}There was a problem hiding this comment.
Ok, I gotcha now. I fixed it to throws instead of catch, and did the same for the other methods. I added throws to the last test for clarity even though it catches all exceptions, as a hint to the reader.
| public void testDeletePage() { | ||
| URL url = new URL("","","",0,false,false); | ||
| try { | ||
| PageLoader loader = PageLoader.deletePage(url); |
There was a problem hiding this comment.
What is the intent of this test?
There was a problem hiding this comment.
Try to see if deletePage() works with an empty URL. Will refactor with a better method name.
There was a problem hiding this comment.
The method has been given a vastly better descriptive name that does not require guesswork.
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Should this be @Test(expected = NullPointerException.class)? Also if that change is made seem like all of the catch blocks could be dropped.
There was a problem hiding this comment.
@kpm1985 do you know if this is still an issue?
| import webindex.core.models.Page; | ||
| import webindex.core.models.URL; | ||
|
|
||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
There's no reason to suppress this warning, since this is easily avoidable. For example, replace:
Page p = null;
PageLoader loader = PageLoader.updatePage(p);with:
PageLoader loader = PageLoader.updatePage((Page) null);There was a problem hiding this comment.
Thank you, fixed.
Added Apache License header Renamed methods with better descriptives Removed unused local variable warnings
|
I wrote the test because I noticed load(Tx, Ctx) doesn't check for null args. The deletePage(URL url) requires url not be null but doesn't care if its a url representing "", ie empty or blank. The updatePage(Page page) method requires the url is not "" or blank/empty but doesn't care if it gets a null page (leading to NPE). |
Improve handling of unexpected exceptions and cast them to the appropriate type
|
I created a separate issue for the refactoring of PageLoader so that it can pass these tests. It can be found in #9 |
Change method signature to eliminate catch blocks
| loader.load(null, null); | ||
| } catch (Exception e) { | ||
| if(e instanceof NullPointerException) | ||
| throw new NullPointerException(e.getMessage()); |
There was a problem hiding this comment.
I don't understand why this is catching and then rethrowing a NPE. Why even catch it?
There was a problem hiding this comment.
Hello Keith, I did that because loader class throws Exception, but I was trying to determine when it's because of a NullPointer to load(tx,ctx) rather than just the exception load already throws, I fixed it and removed it.
|
@kpm1985 the changes are looking good now. So does this test not pass as it is? |
|
That is correct, I just run it in Eclipse and get a couple failing tests. |
|
@keith-turner The two tests that are failing are testUpdatePageWithNullPage and testLoadWithNullTxCtx. Where I think a simple Objects.requireNonNull(page) could do the trick. The second comes from here: https://github.com/apache/fluo-examples/blob/47f1bb42969345b444f5c0e4c8c4a36ec93aab14/webindex/modules/data/src/main/java/webindex/data/fluo/PageLoader.java#L60-L80 And Objects.requireNonNull() could again do the trick. |
|
@keith-turner @ctubbsii Anybody have some feedback? Does this look ok now? |
3 participants
for factory methods and constructors.