Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Endnote importer test #321

Merged
merged 1 commit into from
Jan 1, 2016
Merged

Endnote importer test #321

merged 1 commit into from
Jan 1, 2016

Conversation

chriba
Copy link
Contributor

@chriba chriba commented Nov 11, 2015

No description provided.

@koppor
Copy link
Member

koppor commented Nov 11, 2015

Could you add some real EndNote entries? Currently, there seem only test entries to be tested, but no real-life entries.

Minor comments

@simonharrer
Copy link
Contributor

Please rebase on master so that circleci and codecov will show correct results.

@koppor
Copy link
Member

koppor commented Nov 17, 2015

We discovered, that EndNote is also capable of importing RIS format and that both IEEExplore and Springer export RIS as "EndNote". (See for example http://link.springer.com/chapter/10.1007/978-3-662-44879-3_8 and http://ieeexplore.ieee.org/xpl/articleDetails.jsp?arnumber=4578545).

EndNote imports both: The RISFormat and the EndNote format (es exported by JabRef). I vote for removing the EndNote importer and focus on the RisImporter.

Furthermore, I vote for removing the EndNote exporter and focus on testing the RISExporter.

@koppor
Copy link
Member

koppor commented Nov 17, 2015

@simonharrer
Copy link
Contributor

Just leave it as it is right now. :)

@koppor
Copy link
Member

koppor commented Dec 21, 2015

public void setUp() throws Exception {
if (Globals.prefs == null) {
Globals.prefs = JabRefPreferences.getInstance();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need the if-condition here. Just set the Globals.prefs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chriba
Copy link
Contributor Author

chriba commented Dec 21, 2015

Fixed the mentioned issues and did some more refactoring.

assertEquals("testU", be4.getField("url"));
assertEquals("testV", be4.getField("volume"));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this in 3 different tests.

@tobiasdiez
Copy link
Member

The tests with small test files should be tested by inline strings as described here: #354 (comment)

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 22, 2015
@koppor
Copy link
Member

koppor commented Dec 22, 2015

I agree with using direct equals tests than using bib files. However, tests with full bib files should not vanish.

We told the students to have a look at GVKParserTest.java and do similar testing. 😇

@koppor
Copy link
Member

koppor commented Dec 31, 2015

Coverage 97.90%. I don't see more test cases to turn the the yellow lines into green ones.

If the test cases and test files get speaking names, I'm fine with it. @tobiasdiez Do you agree?

@tobiasdiez
Copy link
Member

Yes, looks good to me 👍
Just a minor point: add assertions that the bibentry-type is correct.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 1, 2016
@chriba
Copy link
Contributor Author

chriba commented Jan 1, 2016

I used be0 because it's the first item in the list, changed it to be.

The Endnote.book.example.enw was exported from a Library, I removed the space.

koppor added a commit that referenced this pull request Jan 1, 2016
@koppor koppor merged commit 15d15f3 into JabRef:master Jan 1, 2016
@chriba chriba deleted the EndnoteImporterTest branch January 1, 2016 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants