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

MedlineImporterTest #547

Merged
merged 2 commits into from
May 23, 2016
Merged

MedlineImporterTest #547

merged 2 commits into from
May 23, 2016

Conversation

bruehldev
Copy link
Contributor

New PR started because the old one was on a wrong branch.

MedlineImporTest + Testfile added

Couldn't cover two exceptions in importEntries.

@oscargus
Copy link
Contributor

BibtexEntry is called BibEntry now. Not sure how the CircleCI tests passes, but that is the reason that the Travis CI tests won't.

@bruehldev
Copy link
Contributor Author

I'm afraid i cant use the BibEntry. Somehow i can't use the libary ("BibEntry cannot be resolved to a type"), but i fetched my upstream.

@oscargus
Copy link
Contributor

Do you have a package net.sf.jabref.model.entry.BibEntry? model/entry/BibEntry.java
I'm more surprised that you still have BibtexEntry.java.

@bruehldev
Copy link
Contributor Author

I updated the branch with BibEntry. We will solve the package problem with our tutor. Thanks Oscar

simonharrer
simonharrer reviewed Dec 21, 2015
View reviewed changes
src/test/java/net/sf/jabref/importer/fileformat/MedlineImporterTest.java
*/
public class MedlineImporterTest {

MedlineImporter medlineImp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use abbreviations. Either use medlineImporter or importer as the context is clear by being in the MedlineImporterTest class.

@mairdl
Copy link
Contributor

mairdl commented Dec 21, 2015

For now I took care of the smaller changes, more changes coming soon

@boceckts
Copy link
Contributor

Added more test files and added testing against bibtex files.

@koppor
Copy link
Member

koppor commented Jan 10, 2016

@boceckts Is this ready for review? Then please add this label?

@boceckts
Copy link
Contributor

Testcoverage is at only ~80% so far for MedlineImporter and at ~90% for MedlineHandler

@koppor koppor changed the title MedlineImporterTest [WIP] MedlineImporterTest Jan 15, 2016
@koppor koppor added the testing label Mar 18, 2016
@stefan-kolb
Copy link
Member

What is the status here? DL was beginning of match.

tschechlovdev
tschechlovdev reviewed Apr 7, 2016
View reviewed changes
src/test/java/net/sf/jabref/importer/fileformat/MedlineImporterTest.java
try (InputStream shouldBeIs = MedlineImporter.class.getResourceAsStream("MedlineImporterArticleIDTest.xml");
InputStream bibIn = MedlineImporter.class.getResourceAsStream("MedlineImporterArticleIDTestBib.bib")) {
List<BibEntry> entries = medlineImporter.importEntries(shouldBeIs, new OutputPrinterToNull());
Assert.assertEquals(1, entries.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

this assertion is not necessary, it is done in BibtexEntryAssert.assertEquals(InputStream, List <BibEntry>)

@mairdl mairdl force-pushed the MedlineImporter branch 2 times, most recently from 634e1c6 to db2ec40 Compare April 13, 2016 14:10
@koppor
Copy link
Member

koppor commented Apr 13, 2016

Please rebase everything on master and create on commit. Description is here: https://github.com/JabRef/jabref/wiki/Tools#rebase-everything-as-one-commit-on-master Please use git gui for your commit. Please do not delete other test files.

There should no commits handling the InspecImporter in this pull request. Inspec has been handled in #356

@matthiasgeiger
Copy link
Member

matthiasgeiger commented Apr 25, 2016 •
edited by koppor
Loading

Yes. And these tests should be included here, I think.

If the tests fail, the failing tests in conjunction with the bugfix can be in another PR.

@koppor
Copy link
Member

koppor commented Apr 25, 2016

The failed tests can be included here with @Ignore("Will be fixed in a future PR") so that there is no huzzle with git, can't they?

@matthiasgeiger
Copy link
Member

Yes, of course and this is an even better solution. 😉

@matthiasgeiger matthiasgeiger removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 25, 2016
@mairdl
Copy link
Contributor

mairdl commented May 2, 2016

After some testing i came to the conclusion that this bug/issue is not a problem in the current development version. At the moment I am struggeling to coming up with the right test case. At the moment I am thinking of using a file.bib wit some pubmed entries and then importing it with the RISimporter and checking if it works properly. But that seems a bit unfitting for the MedlineImporter.

@simonharrer
Copy link
Contributor

What is the current status of this PR?

@simonharrer simonharrer mentioned this pull request May 9, 2016
Merged
3 tasks
@mairdl
Copy link
Contributor

mairdl commented May 10, 2016

waiting for some feedback regardnig the issue #1267 and my comment and also on comments from me regarding the use of a StringJoiner

@tobiasdiez tobiasdiez mentioned this pull request May 10, 2016
Merged
@mairdl mairdl force-pushed the MedlineImporter branch 4 times, most recently from 5b87a0f to 2de4344 Compare May 12, 2016 08:36
@chriba chriba force-pushed the MedlineImporter branch from 2de4344 to b4fc8e1 Compare May 12, 2016 09:42
@mairdl mairdl force-pushed the MedlineImporter branch from b4fc8e1 to 4c3310b Compare May 12, 2016 10:14
@mairdl
Copy link
Contributor

mairdl commented May 12, 2016

@simonharrer your suggested version leads to some problems

@tobiasdiez
Copy link
Member

Since this PR blocks #1207, I would prefer that we merge this as quickly as possible. So just ignore the issue #1267 for the moment.

@simonharrer
Copy link
Contributor

simonharrer commented May 23, 2016 •
edited
Loading

@mairdl hm, then revert it to the state without my suggestion, and rebase. We try to merge this in quickly as it blocks #1207

The remaining issues can be fixed in a separate PR.

@mairdl
Copy link
Contributor

mairdl commented May 23, 2016

ok i will revert it asap

@mairdl mairdl force-pushed the MedlineImporter branch 2 times, most recently from 7b980e2 to 61205bb Compare May 23, 2016 10:43
@mairdl mairdl force-pushed the MedlineImporter branch from fbe6114 to 5fcb6e0 Compare May 23, 2016 11:05
@mairdl
Copy link
Contributor

mairdl commented May 23, 2016

I reverted the changes of StringUtil and added a ignored label to the tests that would fail.

@mairdl mairdl force-pushed the MedlineImporter branch from cfa006e to 6959133 Compare May 23, 2016 11:31
@simonharrer
Copy link
Contributor

Thanks. Please open a new PR then for the fix of the failing test. :)

@simonharrer simonharrer merged commit 4f870e7 into JabRef:master May 23, 2016
@koppor koppor changed the title [WIP] MedlineImporterTest MedlineImporterTest May 30, 2016
@mairdl mairdl deleted the MedlineImporter branch September 14, 2016 13:14
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.