Add testing interface, including a set of capabilities to tests for by DominikVoigt · Pull Request #6687 · JabRef/jabref

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 https://github.com/koppor/jabref/issues/341.6

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 <dominik.ingo.voigt@gmail.com>

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!

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 <dominik.ingo.voigt@gmail.com>

- Add missing assert - Modify test Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

…hange request Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

…AdvancedSearchBasedParserFetcher interface. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

tobiasdiez

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

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

- 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 <dominik.ingo.voigt@gmail.com>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Add related ADR. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Remove URL from Doifetcher test, as it gets cleaned up by DOICleanUp as the URL contains a DOI. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

…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 <dominik.ingo.voigt@gmail.com>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Remove any format conversion where the fetcher format conversion added it. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor

Some small nitpicks, then it should be good to go

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

        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.

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

+         */
+        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).

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

        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.

- Move doPostCleanup back - Move format in ImportCleanup into constructor. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

…ty-tests-for-fetchers

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

- Change title - Fix some typos and punctuation Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Fix indentation issue in CompositeSearchBasedFetcher Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Remove outdated JavaDoc Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

* 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 <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Correct SpringerFetcherTest

0950cb6

- Add missing assert - Modify test Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Rename AdvancedFetcher and AdvancedSearchConfig.java accordingly to c…

6bffc68

…hange request Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Add documentation to the default field

f5f1b5b

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Modify AdvancedSearchBasedParserFetcher interface to be in line with …

7e81cca

…AdvancedSearchBasedParserFetcher interface. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Adapt test of getAdvancedURL

db19e60

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

tobiasdiez requested changes Jul 15, 2020

View reviewed changes

Comment thread src/main/java/org/jabref/logic/importer/fetcher/IEEE.java Outdated

DominikVoigt added 4 commits July 15, 2020 17:40

Integrate change requests.

4065fa5

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Continue integrate format conversion.

de62312

Add related ADR. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Finalize format conversion changes.

6f482a4

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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

a746197

…ty-tests-for-fetchers

Add ADR.

f675e13

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Update ADR. Remove import.

84a9b53

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Remove unused import.

8fa03d5

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Move format conversion from Fetchers into ImportCleanup.

e1b0bf8

Remove any format conversion where the fetcher format conversion added it. Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Revert incorrect change.

d424f88

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Remove debugging print statement.

d6eb086

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Integrate requested changes.

8f3fe0f

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Change test to use ImportCleanup.

056cc93

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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 <dominik.ingo.voigt@gmail.com>

Integrate requested changes

190414f

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Inline variable

a37d29e

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Modify ADR

4b6b54b

- Change title - Fix some typos and punctuation Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Fix compile error

b5d9369

Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Change ADR title

cf2ff45

Fix indentation issue in CompositeSearchBasedFetcher Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Inline some variables

37293c6

Remove outdated JavaDoc Signed-off-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

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

codepurple007 mentioned this pull request Feb 23, 2026

docs: fix link formatting in ADR 0012 #15201

Merged

2 tasks

Uh oh!

Conversation

DominikVoigt commented Jul 13, 2020

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Jul 13, 2020

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tobiasdiez commented Jul 24, 2020

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants