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

Test for InspecImporter #356

Merged
merged 14 commits into from
Apr 7, 2016
Merged

Conversation

mairdl
Copy link
Contributor

@mairdl mairdl commented Nov 17, 2015

Completed code coverage and discovered a bug in BibtexEntryType (BibtetEntryType.getType() ALL_TYPES.get() returns null)

@Test
public void testImportEntries3() {

InputStream inStream = InspecImportTest.class.getResourceAsStream("InspecImportTest4.txt");
Copy link
Contributor

Choose a reason for hiding this comment

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

try-with-resources please to ensure that the input stream is closed.

Assert.assertTrue("InspecImporter isRecognizedFormaTrue test failed",
inspecImp.isRecognizedFormat(inStream));
Assert.assertFalse("InspecImporter isRecognizedFormatFalse test failed",
inspecImp.isRecognizedFormat(inStream));
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a look at #310 for handling the tests for format recognition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I editet the format recognition, so that it equals the one of #310

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I do not see this in the current PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad I understood it the wrong way. I'll fix it in the next commit.

@mairdl
Copy link
Contributor Author

mairdl commented Nov 19, 2015

It seems like my commits have some issues, but I don't get the problem. Can someone help me out?

@oscargus
Copy link
Contributor

I do not think it is your commit that causes problems. For some reason the SpringerLinkFetch test fails.

@koppor
Copy link
Member

koppor commented Nov 24, 2015

@mairdl I think, it would make sense if you squash all commits into a single commit and to a git push -f. You could also do a git rebase master as described at #379 (comment). After having done that, the rest by CircleCI should not fail any more.

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

@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.

If statement not necessary.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 15, 2016
@koppor koppor changed the title Test for InspecImporter [WIP] Test for InspecImporter Jan 15, 2016
@Braunch
Copy link
Contributor

Braunch commented Mar 14, 2016

Is it ready for a merge now?

@tobiasdiez
Copy link
Member

testCompleteBibtexEntryOnJournalPaperImport still has one not addressed comment. Please rename the commit to something more meaning full like Test for InspecImporter. Other than that 👍 for merge.

inStream.close();
assertEquals(1, entries.size());
BibEntry entry = entries.get(0);
assertEquals(BibtexEntryTypes.INPROCEEDINGS.getName().toLowerCase(), entry.getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace this with BibtexEntryAssert.assertEquals(Arrays.ListOf(shouldBeEntry), entries) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure you mean "Arrays.ListOf(shouldBeEntry)"? Because I can't find this method.

Copy link
Member

Choose a reason for hiding this comment

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

Simon meant the Arrays.asList, but Collections.singletonList also works (I think IntelliJ even suggest replacing asList with singletonList)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i try to solve it with Arrays.asList or with Collections.singletonList, I get an error from the assertion because I try to compare a list of BibEntry with a BibEntry.

Copy link
Member

Choose a reason for hiding this comment

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

But entries is a list, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

entries is a BibEntry as suggested in earlier comments

Copy link
Member

Choose a reason for hiding this comment

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

Please reference the comment. You can get the deep link to a comment using the time given at a commit. While scrolling through here, I don't see that comment directly. Please help the reader by clear references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koppor Here you go comment

@koppor
Copy link
Member

koppor commented Mar 18, 2016

@Braunch: I don't know, what you did, but all commits seem to be lost. Please use gitk to investigate and ask someone else (@mairdl?) if he still has a local copy. He should NOT delete his local code. If all of you deleted it, you have to use the hard man git tooling and look for: git reflog is your friend. See for example at http://gitready.com/advanced/2009/01/17/restoring-lost-commits.html.

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

mairdl commented Mar 18, 2016

@koppor I think Chris(braunch) squashed all commits into 1

@koppor
Copy link
Member

koppor commented Mar 18, 2016

@mairdl It looks like follows on my site:

grabbed_20160318-231122

10ffb1c is an empty commit huge commit and 3e4c73c shows a removal of empty lines. Where is the single commit you are talking about?

@mairdl
Copy link
Contributor Author

mairdl commented Mar 18, 2016

@koppor Now i understand what you mean, my bad. I'll contact @Braunch.

@Braunch
Copy link
Contributor

Braunch commented Mar 27, 2016

I hope my last commit fixed that. Otherwise I am doing something wrong with git.

@stefan-kolb
Copy link
Member

Tests are failing, please investigate.

@mairdl
Copy link
Contributor Author

mairdl commented Mar 31, 2016

Now the tests are working

@mairdl mairdl force-pushed the InspecImportTest branch 2 times, most recently from 49ff62c to 5846469 Compare April 3, 2016 18:02
@stefan-kolb
Copy link
Member

@tobiasdiez @koppor @simonharrer Are you happy enough with it now? We can squash the commits via the merge button.


try (InputStream inStream = new ByteArrayInputStream(testInput.getBytes())) {
List<BibEntry> entries = inspecImp.importEntries(inStream, new OutputPrinterToNull());
assertEquals(1, entries.size());
Copy link
Member

Choose a reason for hiding this comment

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

Use BibtexEntryAssert.assertEquals(Collections.singletonList(shouldbeEntry), entries);

@simonharrer
Copy link
Contributor

Interesting that the code coverage decreases according to codecov by 0.85% in a PR which aims to increase test coverage. Very strange.

@oscargus
Copy link
Contributor

oscargus commented Apr 7, 2016

I think it is because the code in this PR is based on an older code base, so fewer tests included and the comparison is against the (at the time of the push) current master. Push again and it would decrease > 2%...

@simonharrer simonharrer merged commit 6671a44 into JabRef:master Apr 7, 2016
@koppor koppor mentioned this pull request Apr 13, 2016
@Braunch Braunch deleted the InspecImportTest branch September 5, 2016 10:04
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.

7 participants