dkpro / dkpro-jwpl

DKPro JWPL (DKPro Java Wikipedia Library) is a free, Java-based application programming interface that facilitates access to all information in Wikipedia.
https://dkpro.github.io/dkpro-jwpl
Apache License 2.0
82 stars 34 forks source link

PageTest - setupWikipedia() fails but Maven Build still succeeds #161

Closed rzo1 closed 6 years ago

rzo1 commented 6 years ago

While investigating https://github.com/dkpro/dkpro-jwpl/issues/160 I found, that the JUnit tests are not run within the API module (at least PageTest is not executed).

Possible reasons are:

  1. The MySQL driver is missing in the test-scope. For this reason, setup of the test fails with org.hibernate.boot.registry.classloading.spi.ClassLoadingException: Unable to load class [com.mysql.jdbc.Driver]

  2. The test-setup silently ignores the fact, that setupWikipedia() failed with an exception. For this reason, potential compatibility issues from upgrading libraries cannot be identified as the Maven Build completes with SUCCESS.

  3. MySQL DB is not accessible for remote developers. This is tackled with https://github.com/dkpro/dkpro-jwpl/issues/2

  4. I think, that Assume.assumeNoException(e); is not suitable in case tests fail with an exception as the corresponding test will be "ignored". The javadoc states Use to assume that an operation completes normally.. In my opinion, the test-cases should fail in case an assumption breaks, so we can be sure, that the code-base is working after an lib upgrade.

tgalery commented 6 years ago

Isn't this related to the fact that the tests wrapped try and catches ? After https://github.com/dkpro/dkpro-jwpl/pull/162 ideally we would have the build failing due to #160 , right ?

rzo1 commented 6 years ago

The reason for this is Assume.assumeNoException(e); instead of logging the exception in the test-setup and proceed with fail(...) to finally fail the test-case.

If we remove Assume.assumeNoException(e); in the code-base in #162, the build will fail due to #160

mawiesne commented 6 years ago

@rzo1 This issue should be fixed once #162 is merged to master. As soon this was done, could you please re-review PageTest and other tests of the code base and tell us, if the latest changes fix your concern reported with this issue?

mawiesne commented 6 years ago

@rzo1 #2 is merged to master now. Please pull/fetch on your local checkout and re-evaluate on the original concerns you addressed in the issue description.

rzo1 commented 6 years ago

The only problematic test remaining is

@Test
    public void testPlainText(){
        String title = "Wikipedia API";
        Page p = null;
        try {
            p = wiki.getPage(title);
        } catch (WikiApiException e) {
            e.printStackTrace();
            fail("A WikiApiException occured while getting the page " + title);
        }

        String text = "Wikipedia API ist die wichtigste Software überhaupt. Wikipedia API.\nNicht zu übertreffen.\nUnglaublich\nhttp://www.ukp.tu-darmstadt.de\nen:Wikipedia API fi:WikipediaAPI";

        try{
            assertEquals(text, p.getPlainText());
        }catch(Exception e){            
        Assume.assumeNoException(e);
        }
    }

This is directly linked to #160

Chaning the test to:

    @Test
    public void testPlainText(){
        String title = "Wikipedia API";
        Page p = null;
        try {
            p = wiki.getPage(title);
        } catch (WikiApiException e) {
            e.printStackTrace();
            fail("A WikiApiException occured while getting the page " + title);
        }

        String text = "Wikipedia API ist die wichtigste Software überhaupt. Wikipedia API.\nNicht zu übertreffen.\nUnglaublich\nhttp://www.ukp.tu-darmstadt.de\nen:Wikipedia API fi:WikipediaAPI";

        try{
            assertEquals(text, p.getPlainText());
        }catch(Exception e){
            fail(e.getMessage());
            //Assume.assumeNoException(e);
        }
    }

uncovers the issue described in #160 .

If #160 is fixed, this issue (#161) can be fixed as well...

FYI: @tgalery #160 is reproducable now...

mawiesne commented 6 years ago

@rzo1 I think, lastest changes in #160 resolve this issue. Please check carefully if this is the case and report back your feedback. Potentially, close this issue once PR #185 is merged.

mawiesne commented 6 years ago

I will close this issue, as @rzo1 reported in PR #185 that it is fixed "en passant". Thus, no further changes in the code are needed.