Add testing interface, including a set of capabilities to tests for #6687

DominikVoigt · 2020-07-13T19:45:06Z

Add capability tests for the fetchers to determine whether they support the tested capability.

This is a part of my bachelor thesis working on JabRef#369.

Here, I am systematically evaluating the capabilities of fetchers. This is a) for documentation in code b) to check whether the fetchers change their capability over time. I accept that tests may fail due to changing external services.

Follow-up PRs will come making use of the new functionality in the UI. This is a rather small PR to easy review (@stefan-kolb ^^). This is also a step towards implementing JabRef#341

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Add capability tests for the fetchers to determine whether they support the tested capability. Signed-off-by: Dominik Voigt <[email protected]>

koppor

Reviewed the code before this PR was submitted. Therefore, approving (while waiting what the automatec checks will say)

…ub.com/DominikVoigt/jabref into DominikVoigt-feat/add-capability-tests-for-fetchers

Siedlerchr · 2020-07-13T20:11:39Z

One of the architecture test fails:
getSearchBasedFetchersReturnsAllFetcherDerivingFromSearchBasedFetcher

Probably because we hide some fetchers for the users

tobiasdiez

Thanks for your PR, this is a nice bachelor project.

I think it's good to get the interfaces right from the start, so I have a few comments concerning the overall structure. But overall the changes are fine, good job!

src/main/java/org/jabref/logic/importer/AdvancedFetcher.java

src/main/java/org/jabref/logic/importer/fetcher/AdvancedSearchConfig.java

src/test/java/org/jabref/logic/importer/fetcher/ArXivTest.java

src/test/java/org/jabref/logic/importer/fetcher/SearchBasedFetcherCapabilityTest.java

src/test/java/org/jabref/logic/importer/fetcher/SpringerFetcherTest.java

bf698ac Create common-market-law-review.csl (JabRef#4910) c962eca Create harvard-prifysgol-caerdydd.csl (JabRef#4922) 0c24e7f Update gewerblicher-rechtsschutz-und-urheberrecht.csl (JabRef#4923) d2ec1a7 Create Tijdschrift-voor-Geneeskunde.csl (JabRef#4907) 5df7250 Update harvard-institut-fur-praxisforschung-de.csl (JabRef#4918) 093fd91 Update universite-de-montreal-apa.csl (JabRef#4916) a3e41d4 Update thieme-german.csl (JabRef#4919) 648765a add DOI to aerosol-science-and-technology.csl (JabRef#4909) bc1ebee Reindent/reorder a8dc18a Fix documentation link for epidemiology & infection aab403a Fix AGLC Newspaper date 4c018d5 Add period between editor and translator (SBL styles) (JabRef#4906) 42f7491 Create geographische-zeitschrift.csl (JabRef#4898) a4002a6 Create german-kunstwissenschaft.csl (JabRef#4896) b01910b Disambiguation of names (JabRef#4895) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: bf698ac

…ub.com/DominikVoigt/jabref into feat/add-capability-tests-for-fetchers

Signed-off-by: Dominik Voigt <[email protected]>

- Add missing assert - Modify test Signed-off-by: Dominik Voigt <[email protected]>

…hange request Signed-off-by: Dominik Voigt <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

…AdvancedSearchBasedParserFetcher interface. Signed-off-by: Dominik Voigt <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

tobiasdiez

Now there are many unrelated changes in this PR. I guess a rebase/merge master should fix this.

src/main/java/org/jabref/logic/importer/fetcher/IEEE.java

Signed-off-by: Dominik Voigt <[email protected]>

- Add IEEE Xplore fetcher test - Move performComplexSearch into the SearchBasedFetcher Interface - Modify TestInterface to define how the objects under test behave - Use performComplexSearch in the TestInterface - Modify ComplexSearchQuery to allow for multiple author and multiple title phrase searches - Change SpringerFetcher from using JournalTitle field instead of Journal field, to be in line with IEEE and ArXiv Fetcher Signed-off-by: Dominik Voigt <[email protected]>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <[email protected]>

Add related ADR. Signed-off-by: Dominik Voigt <[email protected]>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <[email protected]>

Remove URL from Doifetcher test, as it gets cleaned up by DOICleanUp as the URL contains a DOI. Signed-off-by: Dominik Voigt <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <[email protected]>

bf698ac" This reverts commit c7795e9. This hopefully fixes the issue with the csl files. Signed-off-by: Dominik Voigt <[email protected]> # Conflicts: # src/main/resources/csl-styles/common-market-law-review.csl # src/main/resources/csl-styles/dependent/prifysgol-caerdydd-harvard.csl # src/main/resources/csl-styles/geographische-zeitschrift.csl # src/main/resources/csl-styles/kunstakademie-munster.csl # src/main/resources/csl-styles/tijdschrift-voor-geneeskunde.csl

…a995c63..bf698ac"" This reverts commit c8ead6b.

tobiasdiez · 2020-07-24T11:13:39Z

Concerning the biblatex <-> bibtex conversion, please keep in mind that this is not only specific to fetchers, but actually concerns all ways entries can be added to the library (fetcher, import, paste from clipboard, etc).
Moreover, there are other post-process steps (apart from the bibtex conversion) that would ideally be handled, for example, bibtex key generation, adding of "timestamp" and "user" fields etc.
We discussed this already a few times, and the tendency was towards adding a conversion handler that post-processes the output of the fetcher/importer/... and then adds the entries in the database:
#2117 (comment)
#1018 (comment)
Also relevant: #895

Anyway, should we move this to a new PR since I fear otherwise this one here gets to huge to be reviewable?

Signed-off-by: Dominik Voigt <[email protected]>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <[email protected]>

Remove any format conversion where the fetcher format conversion added it. Signed-off-by: Dominik Voigt <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

koppor

Some small nitpicks, then it should be good to go

src/main/java/org/jabref/cli/ArgumentProcessor.java

docs/adr/0013-how-to-provide-fetchers-with-import-format.md

src/main/java/org/jabref/cli/ArgumentProcessor.java

src/main/java/org/jabref/logic/importer/EntryBasedParserFetcher.java

src/main/java/org/jabref/logic/importer/IdBasedParserFetcher.java

src/main/java/org/jabref/logic/importer/fetcher/CiteSeer.java

src/main/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcher.java

src/main/java/org/jabref/logic/importer/fetcher/DoiFetcher.java

koppor · 2020-08-01T20:56:50Z

src/main/java/org/jabref/logic/importer/fetcher/IEEE.java

@@ -115,7 +106,11 @@ private static BibEntry parseJsonRespone(JSONObject jsonEntry, Character keyword
        entry.setField(StandardField.ISBN, jsonEntry.optString("isbn"));
        entry.setField(StandardField.ISSN, jsonEntry.optString("issn"));
        entry.setField(StandardField.ISSUE, jsonEntry.optString("issue"));
-        entry.addFile(new LinkedFile("", Path.of(jsonEntry.optString("pdf_url")), "PDF"));
+        try {
+            entry.addFile(new LinkedFile(new URL(jsonEntry.optString("pdf_url")), "PDF"));


This needs to be discused -> maybe in a separate PR?

The LinkedFile points to a file locally stored. Not to a URL. -- Maybe, just use the URL field of the BibEntry. Or use the URLDownloader to download and store the file. (Needs some investigation how JabRef downloads the file; since JabRef does some rename and folder-sort-magic)

Linked files can also contain an online link.
However in this case I would set the URL field with the pdf url.

I think this is correct: the url for the download of the fulltetx should be added as a linked file.

Did not know this concept. Seemed to be introduced at 5e1adf8. Thus, OK for me.

tobiasdiez

Thanks for your continued work on this! The code looks indeed really good. We can merge after a few nit-picks are fixed.

Afterwards, it would be good if the following things can be addressed as well:

Change direction of flow so that getUrlForQuery calls getComplexQueryURL (so that fetchers only have to override the latter in normal circumstances).
In fact, it should be investigate if performSearch(String) can be completely removed and one only uses the complex query.

docs/adr/0012-handle-different-bibEntry-formats-of-fetchers.md

src/main/java/org/jabref/gui/ClipBoardManager.java

src/main/java/org/jabref/gui/bibtexextractor/BibtexExtractorViewModel.java

src/main/java/org/jabref/logic/importer/ImportCleanup.java

src/main/java/org/jabref/logic/importer/SearchBasedFetcher.java

src/main/java/org/jabref/logic/importer/SearchBasedParserFetcher.java

tobiasdiez · 2020-08-01T22:36:07Z

src/main/java/org/jabref/logic/importer/fetcher/ComplexSearchQuery.java

+         */
+        public ComplexSearchQueryBuilder author(String author) {
+            if (Objects.requireNonNull(author).isBlank()) {
+                throw new IllegalArgumentException("Parameter must not be blank");


Is it really a problem if the user searches for an empty string, e.g. author = ""?

Hey :)
IMHO allowing empty strings for any query parameter does not really make much sense semantically.
I do not see the semantics such an empty string should have:

If the user does not want to specify an author, he should just leave the field null

Otherwise, he tries to searches for documents without authors?

Did I overlook a use case where giving an empty string for a query parameter is useful?

Sounds good! I just wanted to double check.
(We should keep this in the back of our heads when the user interface is implemented. There you would like to show a helping message instead of an error dialog).

src/main/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcher.java

tobiasdiez · 2020-08-01T22:40:41Z

src/main/java/org/jabref/logic/importer/fetcher/IEEE.java

@@ -115,7 +106,11 @@ private static BibEntry parseJsonRespone(JSONObject jsonEntry, Character keyword
        entry.setField(StandardField.ISBN, jsonEntry.optString("isbn"));
        entry.setField(StandardField.ISSN, jsonEntry.optString("issn"));
        entry.setField(StandardField.ISSUE, jsonEntry.optString("issue"));
-        entry.addFile(new LinkedFile("", Path.of(jsonEntry.optString("pdf_url")), "PDF"));
+        try {
+            entry.addFile(new LinkedFile(new URL(jsonEntry.optString("pdf_url")), "PDF"));


I think this is correct: the url for the download of the fulltetx should be added as a linked file.

src/test/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcherTest.java

- Move doPostCleanup back - Move format in ImportCleanup into constructor. Signed-off-by: Dominik Voigt <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <[email protected]>

- Change title - Fix some typos and punctuation Signed-off-by: Dominik Voigt <[email protected]>

Signed-off-by: Dominik Voigt <[email protected]>

Fix indentation issue in CompositeSearchBasedFetcher Signed-off-by: Dominik Voigt <[email protected]>

Remove outdated JavaDoc Signed-off-by: Dominik Voigt <[email protected]>

* upstream/master: (47 commits) Fix copy pasting and delete via menu or key (#6740) Add instructions how to work with fetchers (#6731) Autoinstall extension in chrome (#6442) Delete link after download (#6723) New translations JabRef_en.properties (Portuguese, Brazilian) (#6728) Bump pascalgn/automerge-action from v0.8.5 to v0.9.0 (#6736) Bump byte-buddy-parent from 1.10.13 to 1.10.14 (#6733) Bump mockito-core from 3.4.4 to 3.4.6 (#6734) Bump unirest-java from 3.8.06 to 3.9.00 (#6735) Bump org.beryx.jlink from 2.21.1 to 2.21.2 (#6732) Add testing interface, including a set of capabilities to tests for (#6687) Fix pasting on mac and linux (#6419) Add validation of "AUTHORS" file (#6722) Squashed 'src/main/resources/csl-styles/' changes from cacc4ee..827b986 New Crowdin updates (#6721) Add missing AUTHORs Fix for issue 6639 (#6719) Fix more links Fix link New Crowdin updates (#6718) ...

Add testing interface, including a set of capabilities to tests for

a60bdec

Add capability tests for the fetchers to determine whether they support the tested capability. Signed-off-by: Dominik Voigt <[email protected]>

koppor approved these changes Jul 13, 2020

View reviewed changes

Merge branch 'feat/add-capability-tests-for-fetchers' of https://gith…

4881f7b

…ub.com/DominikVoigt/jabref into DominikVoigt-feat/add-capability-tests-for-fetchers

Remove all interfaces

7c13275

Siedlerchr approved these changes Jul 14, 2020

View reviewed changes

tobiasdiez requested changes Jul 14, 2020

View reviewed changes

github actions and others added 9 commits July 15, 2020 02:11

Merge commit 'c7795e9124d55908b8f074c475d62229972a4edc'

a89db08

Merge branch 'feat/add-capability-tests-for-fetchers' of https://gith…

5ec7ef6

…ub.com/DominikVoigt/jabref into feat/add-capability-tests-for-fetchers

Remove empty lines in tests

ea4b67f

Signed-off-by: Dominik Voigt <[email protected]>

Correct SpringerFetcherTest

0950cb6

- Add missing assert - Modify test Signed-off-by: Dominik Voigt <[email protected]>

Rename AdvancedFetcher and AdvancedSearchConfig.java accordingly to c…

6bffc68

…hange request Signed-off-by: Dominik Voigt <[email protected]>

Add documentation to the default field

f5f1b5b

Signed-off-by: Dominik Voigt <[email protected]>

Modify AdvancedSearchBasedParserFetcher interface to be in line with …

7e81cca

…AdvancedSearchBasedParserFetcher interface. Signed-off-by: Dominik Voigt <[email protected]>

Adapt test of getAdvancedURL

db19e60

Signed-off-by: Dominik Voigt <[email protected]>

tobiasdiez requested changes Jul 15, 2020

View reviewed changes

src/main/java/org/jabref/logic/importer/fetcher/IEEE.java Outdated Show resolved Hide resolved

DominikVoigt added 4 commits July 15, 2020 17:40

Integrate change requests.

4065fa5

Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into feat/add-capabili…

d2a79fd

…ty-tests-for-fetchers

Start Working on format conversion

9673b6c

Signed-off-by: Dominik Voigt <[email protected]>

koppor mentioned this pull request Jul 19, 2020

Using master of forks is dangeours #6699

Closed

DominikVoigt added 8 commits July 20, 2020 16:09

Continue integrate format conversion

e4b1ad1

Signed-off-by: Dominik Voigt <[email protected]>

Continue integrate format conversion.

de62312

Add related ADR. Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into feat/add-capabili…

bb089f4

…ty-tests-for-fetchers

Get it to build. Start correcting tests.

988a275

Signed-off-by: Dominik Voigt <[email protected]>

Work on correcting tests.

a2a26e2

Remove URL from Doifetcher test, as it gets cleaned up by DOICleanUp as the URL contains a DOI. Signed-off-by: Dominik Voigt <[email protected]>

Finalize format conversion changes.

6f482a4

Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into feat/add-capabili…

a746197

…ty-tests-for-fetchers

Add ADR.

f675e13

Signed-off-by: Dominik Voigt <[email protected]>

DominikVoigt added 2 commits July 24, 2020 12:42

Revert "Revert "Squashed 'src/main/resources/csl-styles/' changes from …

1eddd44

…a995c63..bf698ac"" This reverts commit c8ead6b.

DominikVoigt added 8 commits July 26, 2020 11:03

Update ADRs

92c05be

Signed-off-by: Dominik Voigt <[email protected]>

Update ADR. Remove import.

84a9b53

Signed-off-by: Dominik Voigt <[email protected]>

Remove unused import.

8fa03d5

Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into feat/add-capabili…

b86f34e

…ty-tests-for-fetchers

Remove whitespaces and redundant paragraph from ADR.

c694bc1

Signed-off-by: Dominik Voigt <[email protected]>

Move format conversion from Fetchers into ImportCleanup.

e1b0bf8

Remove any format conversion where the fetcher format conversion added it. Signed-off-by: Dominik Voigt <[email protected]>

Revert incorrect change.

d424f88

Signed-off-by: Dominik Voigt <[email protected]>

Remove debugging print statement.

d6eb086

Signed-off-by: Dominik Voigt <[email protected]>

koppor requested changes Aug 1, 2020

View reviewed changes

tobiasdiez requested changes Aug 1, 2020

View reviewed changes

DominikVoigt added 11 commits August 2, 2020 13:44

Integrate requested changes.

f08184e

- Move doPostCleanup back - Move format in ImportCleanup into constructor. Signed-off-by: Dominik Voigt <[email protected]>

Integrate requested changes.

8f3fe0f

Signed-off-by: Dominik Voigt <[email protected]>

Change test to use ImportCleanup.

056cc93

Signed-off-by: Dominik Voigt <[email protected]>

Merge remote-tracking branch 'upstream/master' into feat/add-capabili…

e0fc10a

…ty-tests-for-fetchers

Remove unused bibentry format

1c87c1d

Signed-off-by: Dominik Voigt <[email protected]>

Integrate requested changes

190414f

Signed-off-by: Dominik Voigt <[email protected]>

Inline variable

a37d29e

Signed-off-by: Dominik Voigt <[email protected]>

Modify ADR

4b6b54b

- Change title - Fix some typos and punctuation Signed-off-by: Dominik Voigt <[email protected]>

Fix compile error

b5d9369

Signed-off-by: Dominik Voigt <[email protected]>

Change ADR title

cf2ff45

Fix indentation issue in CompositeSearchBasedFetcher Signed-off-by: Dominik Voigt <[email protected]>

Inline some variables

37293c6

Remove outdated JavaDoc Signed-off-by: Dominik Voigt <[email protected]>

koppor merged commit b506baa into JabRef:master Aug 2, 2020

This was referenced Aug 2, 2020

Fetcher: Think of getUrlForQuery vs. getComplexQueryURL JabRef/jabref-koppor#450

Open

Create a nice ImportHandler JabRef/jabref-koppor#451

Open

Fetcher tests failing #6369

Closed

DominikVoigt deleted the feat/add-capability-tests-for-fetchers branch January 1, 2021 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testing interface, including a set of capabilities to tests for #6687

Add testing interface, including a set of capabilities to tests for #6687

DominikVoigt commented Jul 13, 2020

koppor left a comment

Siedlerchr commented Jul 13, 2020

tobiasdiez left a comment

tobiasdiez left a comment

tobiasdiez commented Jul 24, 2020

koppor left a comment

koppor Aug 1, 2020

Siedlerchr Aug 1, 2020

tobiasdiez Aug 1, 2020

koppor Aug 2, 2020

tobiasdiez left a comment

tobiasdiez Aug 1, 2020

DominikVoigt Aug 2, 2020

tobiasdiez Aug 2, 2020

tobiasdiez Aug 1, 2020

Add testing interface, including a set of capabilities to tests for #6687

Add testing interface, including a set of capabilities to tests for #6687

Conversation

DominikVoigt commented Jul 13, 2020

koppor left a comment

Choose a reason for hiding this comment

Siedlerchr commented Jul 13, 2020

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez commented Jul 24, 2020

koppor left a 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

tobiasdiez left a 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