Fix fetcher tests by Siedlerchr · Pull Request #4216 · JabRef/jabref

Siedlerchr · 2018-07-17T13:40:16Z

Fix fetcher test and fix l10n logging for test

@tobiasdiez could you please check if the zbMath and the MathSciNet tests are still correct?
I am getting 401 as I have no access.

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

remove uncessary logging in l10n which makes fetcher test fail ISBN tests still fail both Fix arxiv tests, eprint now has url in front

tobiasdiez

I've a few remarks that should be fixed before merging. In particular, some of the fetcher need to be changed instead of the test cases...

tobiasdiez · 2018-07-17T13:46:38Z

        if (localizedMessages == null) {
-            // I'm logging this because it should never happen
-            LOGGER.error("Messages are not initialized before accessing key: " + key);
+            // This will happen when running junit tests


Why is this now a problem? While changing some localization-based stuff, I hit this error message sometimes during development and it was helpful to locate the problem. Hence, I would prefer if it could stay.

Yes, but the problem now is that you will hit a NotInitlaizedException in junit. Execute the isbn fetcher test for example and you will see it. It has somehow to do with the fact that the log4j tries to call another appender (GUI based in the end, though it's executed from the test).

The appender are/can be configured separately for the tests, so removing the GUI-appender is worth a try.

Fixed the loging configuration, the xml file had to be renamed

tobiasdiez · 2018-07-17T13:47:40Z

        sliceTheoremPaper.setField("date", "2014-05-09");
        sliceTheoremPaper.setField("abstract", "A general slice theorem for the action of a Fr\\'echet Lie group on a Fr\\'echet manifolds is established. The Nash-Moser theorem provides the fundamental tool to generalize the result of Palais to this infinite-dimensional setting. The presented slice theorem is illustrated by its application to gauge theories: the action of the gauge transformation group admits smooth slices at every point and thus the gauge orbit space is stratified by Fr\\'echet manifolds. Furthermore, a covariant and symplectic formulation of classical field theory is proposed and extensively discussed. At the root of this novel framework is the incorporation of field degrees of freedom F and spacetime M into the product manifold F * M. The induced bigrading of differential forms is used in order to carry over the usual symplectic theory to this new setting. The examples of the Klein-Gordon field and general Yang-Mills theory illustrate that the presented approach conveniently handles the occurring symmetries.");
-        sliceTheoremPaper.setField("eprint", "1405.2249v1");
+        sliceTheoremPaper.setField("eprint", "http://arxiv.org/abs/1405.2249v1");


This is wrong... it should just insert the number (i.e. the fetcher should be fixed not the test)

tobiasdiez · 2018-07-17T13:48:08Z

        expected.setField("date", "2003-07-07");
        expected.setField("abstract", "Multi-electron production is studied at high electron transverse momentum in positron- and electron-proton collisions using the H1 detector at HERA. The data correspond to an integrated luminosity of 115 pb-1. Di-electron and tri-electron event yields are measured. Cross sections are derived in a restricted phase space region dominated by photon-photon collisions. In general good agreement is found with the Standard Model predictions. However, for electron pair invariant masses above 100 GeV, three di-electron events and three tri-electron events are observed, compared to Standard Model expectations of 0.30 \\pm 0.04 and 0.23 \\pm 0.04, respectively.");
-        expected.setField("eprint", "hep-ex/0307015v1");
+        expected.setField("eprint", "http://arxiv.org/abs/hep-ex/0307015v1");


tobiasdiez · 2018-07-17T13:48:25Z

        sliceTheoremPaper.clearField(FieldName.EPRINT);

-        assertEquals(ArXivIdentifier.parse("1405.2249v1"), finder.findIdentifier(sliceTheoremPaper));
+        assertEquals(ArXivIdentifier.parse("http://arxiv.org/abs/1405.2249v1"), finder.findIdentifier(sliceTheoremPaper));


should work just with the id...

Same reason as above, the bibtex code returns the url + id and will not be equal if not.

Well, then the implementation needs be changed to strip the url part ;-)

The strip was already implemented, but it checked for https and not http.

tobiasdiez · 2018-07-17T13:49:29Z

+        when(prefs.getKeywordSeparator()).thenReturn(',');
+    }
+
+    private final LibraryOfCongress fetcher = new LibraryOfCongress(prefs);


initialize prefs and fetcher in setup

tobiasdiez · 2018-07-17T13:51:26Z

    public void testPerformSearchByIdInvalidDoi() throws Exception {
-        Optional<BibEntry> fetchedEntry = fetcher.performSearchById("this.doi.will.fail");
-        assertEquals(Optional.empty(), fetchedEntry);
+        assertThrows(FetcherException.class, () -> fetcher.performSearchById("this.doi.will.fail"));


I'm not sure about our conventions, but whether we throw an exception or return an empty result should be consistent across id fetchers (and it should be added as a general test for all id fetchers (there should be a test class for all id fetchers)). Personally, I would prefer an empty result instead of an exception.

The problem is that our URLDownload throws a FileNotFound Exception on an 404 error, because you are trying to access an input stream of a non existent resource) and thus resulting in a fetcher exception which does not return anything.

I think the default is an empty result if the ID cannot be found or used.

I changed the method in UrlDownload to return an empty stream when a 404 or 400 error (the latter one is thrown by medline) is encountered. I simply log the status response code + url .

Can't you catch the Exception inside the fetcher? I don't like our URLDownload implementation anyway.

Well that was done before but the fetcher exception just encapsulated a file not found exception with no way to getting the underlying http status code.

stefan-kolb

Agree with most of what @tobiasdiez already said

Fix logging configuration Fix eprint field of arxiv Return empty results when 404 or 400 encountered Add logging in that case

stefan-kolb · 2018-07-18T10:51:43Z

-            // remove leading https://arxiv.org/abs/ from abstract url to get arXiv ID
-            String prefix = "https://arxiv.org/abs/";
+            // remove leading http://arxiv.org/abs/ from abstract url to get arXiv ID
+            String prefix = "http://arxiv.org/abs/";


Why no HTTPS anymore? We just changed this?!

I am also in favor of removing both http and https prefixes

The fetcher itself still uses https, but the prefix for the eprint field does not start with https but with http

But we only remove http prefixes not https (if there exists one at a point in time)?!

Added check for both

stefan-kolb · 2018-07-18T10:52:55Z


+    @Test
+    public void findNothing() throws Exception
+    {


Brackets alignment

Siedlerchr · 2018-07-19T16:11:30Z

I fixed all remainig issues. Error response 400 or 404 are logged in the error log.

* upstream/master: single line text fields (#4138) Make it easier to rename and move files (#4200) Fix that swing dialogs are hidden behind main window (#4205) Fixed: #4166 add move & rename file (#4217)

Siedlerchr · 2018-07-23T09:29:26Z

Ebook.de and Google Scholar work again. Chimbori fetcher is broken, but author is noticed and responded that it will be fixed soon.

tobiasdiez

Only a very small remark. Feel free to merge directly after fixing it.

tobiasdiez · 2018-07-23T12:20:42Z

    private static final Logger LOGGER = LoggerFactory.getLogger(ArXiv.class);

    private static final String API_URL = "https://export.arxiv.org/api/query";
+    // remove leading http(s)://arxiv.org/abs/ from abstract url to get arXiv ID


This comment should not be here but in the method below

* upstream/master: Update dependencies (#4231) Update to latest release of richtextfx (#4213) Fix fetcher tests (#4216) single line text fields (#4138) Make it easier to rename and move files (#4200) Fix that swing dialogs are hidden behind main window (#4205) Fixed: #4166 add move & rename file (#4217) # Conflicts: # CHANGELOG.md

* upstream/master: (47 commits) Make attached files relative to the file directory (#4212) execute set visible in swing thread to avoid blocking Fix isbn chimbori test (#4234) Rewrite web search pane in JavaFX (#4203) Update dependencies (#4231) Update to latest release of richtextfx (#4213) Fix fetcher tests (#4216) single line text fields (#4138) Make it easier to rename and move files (#4200) Fix that swing dialogs are hidden behind main window (#4205) Fixed: #4166 add move & rename file (#4217) update gradle plugins and gradlen to 4.9 Update dependencies Remove unnecessary look and feel migration (#4204) Revert threading fix since this sometimes lead to freezes... Update year Fix a few more threading exceptions Refactor Convert CiteSeerX fetcher to new infrastructure (#4185) Make global font size customizable (#4178) ... # Conflicts: # src/main/java/org/jabref/gui/BasePanel.java # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/gui/mergeentries/MergeEntries.java # src/main/resources/l10n/JabRef_en.properties

Fix fetcher tests

f752d86

remove uncessary logging in l10n which makes fetcher test fail ISBN tests still fail both Fix arxiv tests, eprint now has url in front

tobiasdiez requested changes Jul 17, 2018

View reviewed changes

stefan-kolb reviewed Jul 18, 2018

View reviewed changes

Siedlerchr added 2 commits July 18, 2018 11:39

rework fetcher tests

88f6448

Fix logging configuration Fix eprint field of arxiv Return empty results when 404 or 400 encountered Add logging in that case

fix checkstyle

6290a41

stefan-kolb suggested changes Jul 18, 2018

View reviewed changes

Siedlerchr added 2 commits July 18, 2018 14:19

match both https and http prefixes in arxiv

39e8b33

inline prefs in setup method

ca429f3

Siedlerchr mentioned this pull request Jul 18, 2018

ebook.de ISBN to BibTeX is down #4198

Closed

Siedlerchr added 2 commits July 23, 2018 10:47

Merge remote-tracking branch 'upstream/master' into fixtest

e01e445

* upstream/master: single line text fields (#4138) Make it easier to rename and move files (#4200) Fix that swing dialogs are hidden behind main window (#4205) Fixed: #4166 add move & rename file (#4217)

fix ebook.de test record, as there is a new version of that book entry

1eda65b

tobiasdiez requested changes Jul 23, 2018

View reviewed changes

move comment

673dd27

Siedlerchr merged commit d5d67d7 into master Jul 23, 2018

Siedlerchr deleted the fixtest branch July 23, 2018 13:09

Uh oh!

Conversation

Siedlerchr commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

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!

tobiasdiez Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

stefan-kolb left a comment

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!

Siedlerchr commented Jul 19, 2018

Uh oh!

Siedlerchr commented Jul 23, 2018

Uh oh!

tobiasdiez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Siedlerchr commented Jul 17, 2018 •

edited

Loading

tobiasdiez Jul 17, 2018 •

edited

Loading

tobiasdiez left a comment •

edited

Loading