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

Cleanup formatter tests #1159

Merged
merged 5 commits into from
Apr 10, 2016
Merged

Cleanup formatter tests #1159

merged 5 commits into from
Apr 10, 2016

Conversation

koppor
Copy link
Member

@koppor koppor commented Apr 10, 2016

This is a minor cleanup to the formatter tests. Tests covered in FormatterTests were removed from the concrete formatter classes. A comment pointing to the general tests has been added to all concrete test classes.

@koppor koppor force-pushed the cleanup_formatter_tests branch 5 times, most recently from 1454b0c to 5316148 Compare April 10, 2016 11:15
tobiasdiez
tobiasdiez reviewed Apr 10, 2016
View reviewed changes
src/test/java/net/sf/jabref/logic/formatter/CaseChangersTest.java
public class CaseChangersTest {

@Test
public void testChangeCaseLower() {
Assert.assertEquals("", CaseChangers.TO_LOWER_CASE.format(""));
Assert.assertEquals("lower", CaseChangers.TO_LOWER_CASE.format("LOWER"));
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a cleanup PR, could you please also move these test cases to separate test files for the respective CaseChanger (e.g. LowerCaseFormatterTests)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 8e0ce84

@tobiasdiez
Copy link
Member

LGTM 👍 just a few remarks.
For the formatters a better support for parametrized tests like JUnitParams would be nice.

@koppor koppor force-pushed the cleanup_formatter_tests branch 2 times, most recently from 4f8fe59 to 529bdab Compare April 10, 2016 11:53
@koppor koppor added this to the v3.3 milestone Apr 10, 2016
@koppor
Copy link
Member Author

koppor commented Apr 10, 2016

I want to get this into v3.3 as i have another commit based on that, which adds getExampleInput() to Formatters to improve #1050

@koppor koppor mentioned this pull request Apr 10, 2016
Merged
3 tasks
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 10, 2016
@simonharrer
Copy link
Contributor

LGTM as it is.

@koppor
Formatters tests: Remove methods already covered by FormatterTest
feecdc8
@koppor
Sort the formatters in FormatterTest alphabetically to easy manual ch…
6873b71 
…ecking
@koppor
Fix package of test NormalizePagesFormatter
1e587ef
@koppor
Unify formatter tests
aa8398d 
 - No @before, because JUnit initializes the test class at each @test
 - No @after setting the variable to null, because Javas auto cleanup of variables takes care of this, too
 - Use "formatter" as class variable name (instead of other names and initialization within the class)
@koppor
Replace CaseChangersTest by separate test classes
d7cb2bf
@koppor
Copy link
Member Author

koppor commented Apr 10, 2016

I rebased onto master

@koppor koppor merged commit 541c017 into master Apr 10, 2016
@koppor koppor deleted the cleanup_formatter_tests branch April 10, 2016 22:33
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
v3.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@koppor @tobiasdiez @simonharrer