#183 - Raise test coverage of 'Category' (api) class to at least 75% (line & branch) #184

mawiesne · 2018-07-16T18:35:40Z

Adds several test cases in CategoryTest which lifts the coverage for class Page to
98% (line), 86% (branch) -> checked via JaCoCo report of API testsuite run.
Adds test cases for CategoryTitleComparator and PageTitleComparator.
Bonus: Adds new MetaDataTest class with 89% line coverage.
Removes deprecated method Set<Page> getPages() from Category class as nobody used it any way.
Removes dead code (commented and unused/untested) from Category.
Reduces code duplication in Category: getArticleIds() vs. __getPages().
Improves some JavaDoc here and there.
Fixes several misspellings throughout the whole code base of the term occured which should read as occurred (double r).

This addition of tests helps us to sleep well when things get changed in core parts of the API.

Please review and merge this PR.

…(line & branch) - Adds several test cases in `CategoryTest` which lifts the coverage for class Page to 98% (line), 86% (branch) -> checked via JaCoCo report of API testsuite run. - Adds test cases for `CategoryTitleComparator` and `CategoryTitleComparator`. - Bonus: Adds new `MetaDataTest` class with 89% line coverage. - Removes deprecated method `Set<Page> getPages()` from `Category` class as nobody used it any way. - Removes dead code (commented and unused/untested) from `Category` - Reduces code duplication in `Category`: getArticleIds() vs. __getPages() - Improves some JavaDoc here and there - Fixes several misspellings throughout the whole code base of the term _occured_ which should read as _occurred (double r).

ukp-svc-jenkins · 2018-07-16T18:45:47Z

reckart · 2018-07-17T08:57:21Z

de.tudarmstadt.ukp.wikipedia.api/src/main/java/de/tudarmstadt/ukp/wikipedia/api/Category.java

@@ -134,7 +134,7 @@ private void createCategory(Title title) throws WikiPageNotFoundException {
     * This returns the internal id. Do not confuse this with the pageId.
     * @return Returns the internal id.
     */
-    public long __getId() {
+    long __getId() {


Is there a specific reason to downgrade this from public to the default scope?

There is a reason behind this:
I consider the internal, Hibernate/DB specific ID as an internal detail of the DB-access layer. Thus, it should not be exposed to other parts of the application, i.e. no code should rely on accessing the internal database object identifier / primary key. There are other stable data-identifying IDs, such as pageId. This idea follows the principle of information hiding.

Therefore, I lowered the visibility from public and/or protected to package private. With all code compiling and all tests passing, I'm confident that nothing will break or suffer from "pain" related to that architectural change.

Personally, I'm not very used to using package private things. I have sometimes tried using it to make methods accessible for testing, but very rarely. So I understand the concept of information hiding, but haven't found package private to be very useful for me. Which doesn't mean that it is not useful.

On the other hand, I have seen people often to simple forget to put visibility markers. Unfortunately, Java does not have an explicit marker for package private. I would suggest to put a line comment there if package private is being used intentionally and explain the rationale (e.g. framework-internal use) or maybe even to introduce a custom annotation like @Internal just to make the intention explicit and to avoid future developers to think that the modifier has simply been forgotten.

Good point. I will leave a comment on the respective pieces of code and push that with the next commit.

reckart · 2018-07-17T08:57:45Z

de.tudarmstadt.ukp.wikipedia.api/src/main/java/de/tudarmstadt/ukp/wikipedia/api/Category.java

-        Set<Integer> tmpSet = new HashSet<Integer>(hibernateCategory.getPages());
-        session.getTransaction().commit();
-        return tmpSet;
+    Set<Integer> __getPages() {


Is there a specific reason to downgrade this from public to the default scope?

In this case the reasons is:
No piece of code from "external" packages used it, merely internal consumers. Moreover the "__" in front of getPages() indicates the internal use, I guess.

If there is a necessity to change it back, I can do that. Otherwise I suggest to leave it like that, as it is "internal" API here.

reckart · 2018-07-17T08:58:58Z

de.tudarmstadt.ukp.wikipedia.api/src/main/java/de/tudarmstadt/ukp/wikipedia/api/MetaData.java

 	 */
-	public long getId()
+	long getId()


Is there a specific reason to downgrade this from public to the default scope?

see other comment on this topic.

reckart · 2018-07-17T09:00:16Z

de.tudarmstadt.ukp.wikipedia.api/src/main/java/de/tudarmstadt/ukp/wikipedia/api/Page.java

@@ -244,19 +244,17 @@ private void fetchByTitle(Title pTitle, boolean useExactTitle)
 	/**
 	 * @return Returns the id.
 	 */
-	protected long __getId()
+	long __getId()


Is there a specific reason to downgrade this from protected to the default scope?

see other comment on this topic.

reckart · 2018-07-17T09:01:03Z

...darmstadt.ukp.wikipedia.api/src/test/java/de/tudarmstadt/ukp/wikipedia/api/CategoryTest.java

@@ -17,21 +17,24 @@
 *******************************************************************************/
 package de.tudarmstadt.ukp.wikipedia.api;

-import static org.junit.Assert.assertEquals;


Please avoid star imports ;)

I don't see a * import at this line here, as the comment is on a deletion of a line.

reckart · 2018-07-17T09:01:13Z

...darmstadt.ukp.wikipedia.api/src/test/java/de/tudarmstadt/ukp/wikipedia/api/CategoryTest.java

 import org.junit.BeforeClass;
 import org.junit.Test;

 import de.tudarmstadt.ukp.wikipedia.api.exception.WikiApiException;
 import de.tudarmstadt.ukp.wikipedia.api.exception.WikiTitleParsingException;

+import static org.junit.Assert.*;


Please avoid star imports ;)

will be fixed with next commit.

reckart · 2018-07-17T09:07:30Z

...darmstadt.ukp.wikipedia.api/src/test/java/de/tudarmstadt/ukp/wikipedia/api/CategoryTest.java

+                isIds.add(sibling.getPageId());
+            }
+            assertEquals("siblings",expectedPageIds,isIds);
+        } catch (WikiApiException e) {


General comment: there is no need to catch exceptions and handle them by failing explicitly. Just make the unit test method declare throws Exception.

see other comment on Exceptions/Tests.

reckart · 2018-07-17T09:08:17Z

...darmstadt.ukp.wikipedia.api/src/test/java/de/tudarmstadt/ukp/wikipedia/api/MetaDataTest.java

+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.junit.Assert.*;


Please avoid star imports ;) (I'm not going to make more of these line comments - it is a general comment).

Good point. Will keep an eye on it in future commits. IDE had an option on which automatically optimized imports like this.

reckart · 2018-07-17T09:08:37Z

...darmstadt.ukp.wikipedia.api/src/test/java/de/tudarmstadt/ukp/wikipedia/api/MetaDataTest.java

+        try {
+            wiki = new Wikipedia(db);
+        } catch (Exception e) {
+            fail("Wikipedia could not be initialized: " + e.getLocalizedMessage());


Just throw the exception, even in setup methods.

I can't change it in this PR, as this needs a separate issue that only reflects this particular concern.

reckart · 2018-07-17T09:11:37Z

...darmstadt.ukp.wikipedia.api/src/test/java/de/tudarmstadt/ukp/wikipedia/api/MetaDataTest.java

+    @Test
+    public void testGetNumberOfPages() {
+        long numberOfPages = metaData.getNumberOfPages();
+        assertTrue(numberOfPages > 0);


Another general comment: You might want to take a look at assertj - I was recently introduced to it myself and find that after an initial learning curve, it really facilitates writing tests and reduces the number of asserts - or said differently - its asserts are more powerful and comprehensive checking more stuff with a single assert than the ones provided by JUnit.

That's a good pointer. Will look into it, yet again: separate issue/PR for these enhancements.

@reckart

…(line & branch) - Addresses comments by @reckart in PR #184

ukp-svc-jenkins · 2018-07-17T10:08:06Z

reckart · 2018-07-17T11:54:29Z

Note: the coverage information for master now gets properly reported :)

mawiesne · 2018-07-17T11:58:12Z

That looks pretty fine now and is more helpful than before, indeed.

…se-coverage-of-Category-above-75 * Conflicts: * de.tudarmstadt.ukp.wikipedia.api/src/test/resources/db/wikiapi_test.script

…-above-75

ukp-svc-jenkins · 2018-07-17T12:43:10Z

ukp-svc-jenkins · 2018-07-17T13:03:27Z

mawiesne added 🆕Enhancement test labels Jul 16, 2018

mawiesne added this to the 1.2.0 milestone Jul 16, 2018

mawiesne self-assigned this Jul 16, 2018

mawiesne requested review from reckart and daxenberger July 16, 2018 18:35

reckart requested changes Jul 17, 2018

View reviewed changes

#183 - Raise test coverage of 'Category' (api) class to at least 75% …

1d44163

…(line & branch) - Addresses comments by @reckart in PR #184

reckart approved these changes Jul 17, 2018

View reviewed changes

mawiesne and others added 2 commits July 17, 2018 14:15

Merge remote-tracking branch 'origin/master' into enhancement/183-rai…

f4be820

…se-coverage-of-Category-above-75 * Conflicts: * de.tudarmstadt.ukp.wikipedia.api/src/test/resources/db/wikiapi_test.script

Merge branch 'master' into enhancement/183-raise-coverage-of-Category…

5c05ea8

…-above-75

mawiesne merged commit a6b26da into master Jul 17, 2018

mawiesne deleted the enhancement/183-raise-coverage-of-Category-above-75 branch July 17, 2018 13:12

mawiesne mentioned this pull request Jul 17, 2018

Raise test coverage of 'Category' (api) class to at least 75% (line & branch) #183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#183 - Raise test coverage of 'Category' (api) class to at least 75% (line & branch) #184

#183 - Raise test coverage of 'Category' (api) class to at least 75% (line & branch) #184

mawiesne commented Jul 16, 2018

ukp-svc-jenkins commented Jul 16, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

reckart Jul 17, 2018

mawiesne Jul 17, 2018

ukp-svc-jenkins commented Jul 17, 2018

reckart commented Jul 17, 2018

mawiesne commented Jul 17, 2018

ukp-svc-jenkins commented Jul 17, 2018

ukp-svc-jenkins commented Jul 17, 2018

#183 - Raise test coverage of 'Category' (api) class to at least 75% (line & branch) #184

#183 - Raise test coverage of 'Category' (api) class to at least 75% (line & branch) #184

Conversation

mawiesne commented Jul 16, 2018

ukp-svc-jenkins commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ukp-svc-jenkins commented Jul 17, 2018

reckart commented Jul 17, 2018

mawiesne commented Jul 17, 2018

ukp-svc-jenkins commented Jul 17, 2018

ukp-svc-jenkins commented Jul 17, 2018