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.

Sign up for GitHub

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

Jump to bottom

SilverPlatterImporterTest #510

Merged
merged 1 commit into from
Apr 7, 2016
Merged

SilverPlatterImporterTest #510

merged 1 commit into from
Apr 7, 2016

Conversation

tschechlovdev
Copy link
Contributor

No description provided.

@oscargus
Copy link
Contributor

Seems to be some line ending issues here as well.

@koppor
Copy link
Member

koppor commented Dec 17, 2015

Please do the usual git fetch --all --prune, git merge upstream/master, git reset upstream/master, forced push (via git gui) cycle to generate one commit. This eases reviewing. We do NOT have the man power to review 5 or more separate commits.

This should also solve the failing build.

@tschechlovdev tschechlovdev force-pushed the SilverPlatterImporterTest branch 3 times, most recently from 11ce039 to 65e35bb Compare December 21, 2015 14:44
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 21, 2015
@koppor
Copy link
Member

koppor commented Dec 21, 2015

Coverage 90,74%. Please try to increase it.

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

koppor commented Dec 27, 2015

Coverage is now 96,22%. The missed lines are mostly for incomplete entries (pages and title in chapters). Think, it's OK to miss them.

tobiasdiez
tobiasdiez reviewed Dec 28, 2015
View reviewed changes
src/test/java/net/sf/jabref/importer/fileformat/SilverPlatterImporterTest.java

@Before
public void setUp() throws Exception {
if (Globals.prefs == null) {
Copy link
Member

Choose a reason for hiding this comment

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

The if statement is not necessary.

@tschechlovdev tschechlovdev changed the title Silver platter importer test SilverPlatterImporterTest Jan 7, 2016
@koppor koppor changed the title SilverPlatterImporterTest [WIP] SilverPlatterImporterTest Jan 15, 2016
@tschechlovdev tschechlovdev force-pushed the SilverPlatterImporterTest branch 3 times, most recently from 00bbd96 to 275b143 Compare January 25, 2016 15:58
@tschechlovdev tschechlovdev changed the title [WIP] SilverPlatterImporterTest SilverPlatterImporterTest Jan 25, 2016
@tschechlovdev tschechlovdev force-pushed the SilverPlatterImporterTest branch 3 times, most recently from 506515f to 81ded75 Compare January 26, 2016 17:05
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 15, 2016
tobiasdiez
tobiasdiez reviewed Feb 15, 2016
View reviewed changes
src/test/resources/net/sf/jabref/importer/fileformat/SilverPlatterImporterTest1.bib
author = {Supek-S and Aine-CJ},
title = {Simulation studies of multiple dipole neuromagnetic source localization, model order and limits of source resolution. },
year = {1993},
volume = {40(6) },
Copy link
Member

Choose a reason for hiding this comment

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

Volume should be just 40(6) without the space (i.e add trim() in importer)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally 40(6) should be split to volume={40} and issue={6}.

@stefan-kolb stefan-kolb mentioned this pull request Mar 30, 2016
Closed
@stefan-kolb
Copy link
Member

What is the status here?

@tschechlovdev tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 6, 2016
@tschechlovdev
Copy link
Contributor Author

I'm waiting for feedback, wether the test is ok now.

tobiasdiez
tobiasdiez reviewed Apr 6, 2016
View reviewed changes
src/test/java/net/sf/jabref/importer/fileformat/SilverPlatterImporterTest.java
Assert.assertEquals("many-much-more-i don't know", entries.get(0).getField("keywords"));
Assert.assertEquals("Gymnasium Unterrieden", entries.get(0).getField("school"));
Assert.assertEquals("Supek-S and Aine-CJ", entries.get(0).getField("editor"));
Assert.assertEquals("IEEE Trans Biomed Eng", entries.get(0).getField("journal"));
Copy link
Member

Choose a reason for hiding this comment

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

Better use the BibTexAssert.assertEquals(Collections.singletonList(shouldBeEntry), entries). See the other PRs

@tobiasdiez
Copy link
Member

LGTM 👍 just some very small comments

@tschechlovdev tschechlovdev removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 6, 2016
@tschechlovdev tschechlovdev changed the title SilverPlatterImporterTest [WIP]SilverPlatterImporterTest Apr 6, 2016
@tschechlovdev
Copy link
Contributor Author

I've added the comments.
But the tests fail, because of an AssertionError in testImportEntries2(): expected <1> but was <0>.
This error comes from BibtexEntryAssert.assertEquals(). But I checked wether the size of the list is 1 with Assert.assertEquals() and that seems to be ok. So I don't know why this error is comming.

tobiasdiez
tobiasdiez reviewed Apr 6, 2016
View reviewed changes
src/test/resources/net/sf/jabref/importer/fileformat/SilverPlatterImporterTest2.bib
keywords = {many-much-more-i don't know},
school = {Gymnasium Unterrieden},
editor = {Supek-S and Aine-CJ},
title = {imulation studies of multiple dipole neuromagnetic "\},
Copy link
Member

Choose a reason for hiding this comment

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

why is the rest of the title deleted in comparison to the txt file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The title will only be saved till an " in comes. The code herefore is
int inPos = title.indexOf("\" in "); if (inPos > 1) { h.put("title", title.substring(1, inPos)); }
in the SilverplatterImporter.
If there isn't " in in the title then the whole title is saved.

@tobiasdiez
Copy link
Member

The corresponding line is Assert.assertEquals(1, result.getDatabase().getEntryCount()); so there is a problem while parsing the bib file and not of the importer. Does opening the bibfile in JabRef works?

@tschechlovdev
Copy link
Contributor Author

No it doesn't works. It says, that no entries were found.

@tschechlovdev
Copy link
Contributor Author

I found the problem. Now the import of the bib file works. I also changed the test to a parameterized test.

@tschechlovdev tschechlovdev changed the title [WIP]SilverPlatterImporterTest SilverPlatterImporterTest Apr 6, 2016
@tschechlovdev tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 6, 2016
simonharrer
simonharrer reviewed Apr 7, 2016
View reviewed changes
src/test/java/net/sf/jabref/importer/fileformat/SilverPlatterImporterTest.java
@Test
public final void testIsRecognizedFormat() throws Exception {
try (InputStream stream = SilverPlatterImporterTest.class
.getResourceAsStream(txtName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong indent

@simonharrer
Copy link
Contributor

Except my very minor comments 👍 LGTM

@tschechlovdev
Copy link
Contributor Author

I adressed the comments and squashed all commits into one commit.

@simonharrer simonharrer merged commit d8ae5d7 into JabRef:master Apr 7, 2016
@simonharrer
Copy link
Contributor

🏆

@tschechlovdev tschechlovdev deleted the SilverPlatterImporterTest branch April 7, 2016 09:10
@koppor koppor mentioned this pull request Aug 6, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tschechlovdev @oscargus @koppor @tobiasdiez @stefan-kolb @simonharrer