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

Refactoring existing unit tests #7695

Closed
wants to merge 3 commits into from
Closed

Refactoring existing unit tests #7695

wants to merge 3 commits into from

Conversation

Davfon
Copy link
Contributor

@Davfon Davfon commented May 3, 2021

This pull request contributes to issue #6207, which is to add more unit tests or improve existing ones.

Slightly refactored Tests:

  • BibDatabaseDiffTest
  • TestVM
  • FieldFormatterCleanupTest
  • OOBibStyleTest
  • Change in CHANGELOG.md described in a way that is understandable for the average user (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.

Davfon added 3 commits May 2, 2021 12:04
@Davfon
possible improvements for unit tests (based on test smells)
abe8b61 
BibDatabaseDiffTest: Assertion Roulette -> provide message to pinpoint error

TestVM: Assertion Roulette / Magic Numbers -> provide message to pinpoint error, give numbers meaningful names

FieldFormatterCleanupTest: Assertion Roulette -> provide message to pinpoint error (x2)

OOBibStyleTest: Assertion Roulette -> provide message to pinpoint error (x3)

information about test smells: https://testsmells.org/pages/testsmells.html
@Davfon
Make numbers constants
3f13c9c
@Davfon
Merge remote-tracking branch 'upstream/main' into a4-df2
4a9060c
koppor
koppor requested changes May 3, 2021
View reviewed changes
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Thank you for continuing on this.

IMHO the explanations do not help. The output of JUnit is already enough.

I will close the PR.

Please focus on improving the tests by a) adding new test cases or b) converting to paramterized tests.

src/test/java/org/jabref/logic/bibtex/comparator/BibDatabaseDiffTest.java
Comment on lines -21 to +24
assertEquals(Optional.empty(), diff.getPreambleDifferences());
assertEquals(Optional.empty(), diff.getMetaDataDifferences());
assertEquals(Collections.emptyList(), diff.getBibStringDifferences());
assertEquals(Collections.emptyList(), diff.getEntryDifferences());
assertEquals(Optional.empty(), diff.getPreambleDifferences(), "Expected preamble differences to be Optional.empty()");
assertEquals(Optional.empty(), diff.getMetaDataDifferences(), "Expected metadata differences to be Optional.empty()");
assertEquals(Collections.emptyList(), diff.getBibStringDifferences(), "Expected bibstring differences to be Collections.emptyList()");
assertEquals(Collections.emptyList(), diff.getEntryDifferences(), "Expected entry differences to be Collections.emptyList()");
Copy link
Member

Choose a reason for hiding this comment

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

Please undo these changes.

Please convert to paramterized test.

src/test/java/org/jabref/logic/bst/TestVM.java
Comment on lines +54 to +64
final int EXPECTED_NUMBER_OF_STRINGS = 2;
final int EXPECTED_NUMBER_OF_INTEGERS = 7;
final int EXPECTED_NUMBER_OF_ENTRIES = 1;
final int EXPECTED_NUMBER_OF_FIELDS = 5;
final int EXPECTED_NUMBER_OF_FUNCTIONS = 38;

assertEquals(EXPECTED_NUMBER_OF_STRINGS, vm.getStrings().size(), "Expected getStrings() to return a map of size " + EXPECTED_NUMBER_OF_STRINGS);
assertEquals(EXPECTED_NUMBER_OF_INTEGERS, vm.getIntegers().size(), "Expected getIntegers() to return a map of size " + EXPECTED_NUMBER_OF_INTEGERS);
assertEquals(EXPECTED_NUMBER_OF_ENTRIES, vm.getEntries().size(), "Expected getEntries() to return a list of size " + EXPECTED_NUMBER_OF_ENTRIES);
assertEquals(EXPECTED_NUMBER_OF_FIELDS, vm.getEntries().get(0).fields.size(), "Expected entry to have " + EXPECTED_NUMBER_OF_FIELDS + " fields");
assertEquals(EXPECTED_NUMBER_OF_FUNCTIONS, vm.getFunctions().size(), "Expected getFunctions() to return a map of size " + EXPECTED_NUMBER_OF_FUNCTIONS);
Copy link
Member

Choose a reason for hiding this comment

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

This is less readable.

src/test/java/org/jabref/logic/cleanup/FieldFormatterCleanupTest.java
Comment on lines -45 to +51
assertEquals(fieldMap.get(StandardField.TITLE).toUpperCase(), entry.getField(StandardField.TITLE).get());
assertEquals(fieldMap.get(StandardField.BOOKTITLE).toUpperCase(), entry.getField(StandardField.BOOKTITLE).get());
assertEquals(fieldMap.get(StandardField.YEAR).toUpperCase(), entry.getField(StandardField.YEAR).get());
assertEquals(fieldMap.get(StandardField.MONTH).toUpperCase(), entry.getField(StandardField.MONTH).get());
assertEquals(fieldMap.get(StandardField.ABSTRACT).toUpperCase(), entry.getField(StandardField.ABSTRACT).get());
assertEquals(fieldMap.get(StandardField.DOI).toUpperCase(), entry.getField(StandardField.DOI).get());
assertEquals(fieldMap.get(StandardField.ISSN).toUpperCase(), entry.getField(StandardField.ISSN).get());
assertEquals(fieldMap.get(StandardField.TITLE).toUpperCase(), entry.getField(StandardField.TITLE).get(), "Title was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.TITLE).toUpperCase() + " But was: " + entry.getField(StandardField.TITLE).get());
assertEquals(fieldMap.get(StandardField.BOOKTITLE).toUpperCase(), entry.getField(StandardField.BOOKTITLE).get(), "Booktitle was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.BOOKTITLE).toUpperCase() + " But was: " + entry.getField(StandardField.BOOKTITLE).get());
assertEquals(fieldMap.get(StandardField.YEAR).toUpperCase(), entry.getField(StandardField.YEAR).get(), "Year was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.YEAR).toUpperCase() + " But was: " + entry.getField(StandardField.YEAR).get());
assertEquals(fieldMap.get(StandardField.MONTH).toUpperCase(), entry.getField(StandardField.MONTH).get(), "Month was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.MONTH).toUpperCase() + " But was: " + entry.getField(StandardField.MONTH).get());
assertEquals(fieldMap.get(StandardField.ABSTRACT).toUpperCase(), entry.getField(StandardField.ABSTRACT).get(), "Abstract was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.ABSTRACT).toUpperCase() + " But was: " + entry.getField(StandardField.ABSTRACT).get());
assertEquals(fieldMap.get(StandardField.DOI).toUpperCase(), entry.getField(StandardField.DOI).get(), "DOI was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.DOI).toUpperCase() + " But was: " + entry.getField(StandardField.DOI).get());
assertEquals(fieldMap.get(StandardField.ISSN).toUpperCase(), entry.getField(StandardField.ISSN).get(), "ISSN was not cleaned up correctly. Expected: " + fieldMap.get(StandardField.ISSN).toUpperCase() + " But was: " + entry.getField(StandardField.ISSN).get());
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the change

@koppor koppor closed this May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koppor koppor koppor requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Davfon @koppor