Initial PageLoader Test ClassInitial test for the PageLoader class that covers null and empty checks by kennethmcfarland · Pull Request #8 · apache/accumulo-fluo-examples

kennethmcfarland · 2019-03-08T00:40:18Z

for factory methods and constructors.

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

keith-turner · 2019-03-15T14:52:35Z

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

+@SuppressWarnings("unused")
+public class PageLoaderTest {
+
+	@Test


Should this be @Test(expected = NullPointerException.class)?

@kpm1985 do you know if this is still an issue?

keith-turner · 2019-03-15T14:52:55Z

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

+	public void testDeleteFailsWithNullUrl() {
+		try {
+			PageLoader loader = PageLoader.deletePage(null);
+		} catch (MalformedURLException e) {


Can this catch be dropped?

I don't think so because of this method signature:

https://github.com/apache/fluo-examples/blob/47f1bb42969345b444f5c0e4c8c4a36ec93aab14/webindex/modules/data/src/main/java/webindex/data/fluo/PageLoader.java#L52

If the exception is not expected you can make the test throw exception. If it is actually thrown then the test will fail.

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.

I was thinking of the following.

@Test(expected = NullPointerException.class) public void testDeletePageThrowsNPEifNullURL() throws MalformedURLException { PageLoader.deletePage((URL)null); }

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.

keith-turner · 2019-03-15T14:53:08Z

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

+	public void testDeletePage() {
+		URL url = new URL("","","",0,false,false);
+		try {
+			PageLoader loader = PageLoader.deletePage(url);


What is the intent of this test?

Try to see if deletePage() works with an empty URL. Will refactor with a better method name.

The method has been given a vastly better descriptive name that does not require guesswork.

keith-turner · 2019-03-15T14:54:00Z

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

+		}
+	}
+
+	@Test


Should this be @Test(expected = NullPointerException.class)? Also if that change is made seem like all of the catch blocks could be dropped.

@kpm1985 do you know if this is still an issue?

ctubbsii · 2019-03-15T15:10:31Z

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

+import webindex.core.models.Page;
+import webindex.core.models.URL;
+
+@SuppressWarnings("unused")


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);

Thank you, fixed.

Added Apache License header Renamed methods with better descriptives Removed unused local variable warnings

kennethmcfarland · 2019-03-15T22:16:05Z

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

kennethmcfarland · 2019-03-16T00:09:52Z

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

keith-turner · 2019-03-19T20:05:51Z

webindex/modules/data/src/test/java/webindex/data/fluo/PageLoaderTest.java

+			loader.load(null, null);
+		} catch (Exception e) {
+			if(e instanceof NullPointerException)
+				throw new NullPointerException(e.getMessage());


I don't understand why this is catching and then rethrowing a NPE. Why even catch it?

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.

keith-turner · 2019-03-19T20:50:14Z

@kpm1985 the changes are looking good now. So does this test not pass as it is?

kennethmcfarland · 2019-04-03T23:47:33Z

That is correct, I just run it in Eclipse and get a couple failing tests.

kennethmcfarland · 2019-04-04T21:42:59Z

@keith-turner The two tests that are failing are testUpdatePageWithNullPage and testLoadWithNullTxCtx.
The first failing test comes from here https://github.com/apache/fluo-examples/blob/47f1bb42969345b444f5c0e4c8c4a36ec93aab14/webindex/modules/data/src/main/java/webindex/data/fluo/PageLoader.java#L44-L50

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.

kennethmcfarland · 2019-04-28T23:59:08Z

@keith-turner @ctubbsii Anybody have some feedback? Does this look ok now?

Initial test for the PageLoader class that covers null and empty checks

8ee6023

for factory methods and constructors.

keith-turner self-assigned this Mar 12, 2019

keith-turner requested changes Mar 15, 2019

View reviewed changes

ctubbsii reviewed Mar 15, 2019

View reviewed changes

Refactoring of PageLoaderTest

80e1033

Added Apache License header Renamed methods with better descriptives Removed unused local variable warnings

Update Exception Handling

216dccd

Improve handling of unexpected exceptions and cast them to the appropriate type

kennethmcfarland mentioned this pull request Mar 16, 2019

Webdndex PageLoader refactoring #9

Open

Update Exception Handling

7d94a3c

Change method signature to eliminate catch blocks

keith-turner reviewed Mar 19, 2019

View reviewed changes

Update exception handling method signature

abd15cc

ctubbsii changed the base branch from master to main August 11, 2020 19:29

+              		}
+              	}
+              	@Test

Conversation

kennethmcfarland commented Mar 8, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennethmcfarland commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennethmcfarland commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner commented Mar 19, 2019

Uh oh!

kennethmcfarland commented Apr 3, 2019

Uh oh!

kennethmcfarland commented Apr 4, 2019

Uh oh!

kennethmcfarland commented Apr 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kennethmcfarland commented Mar 15, 2019 •

edited

Loading

kennethmcfarland commented Mar 16, 2019 •

edited

Loading