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

Added branch coverage tool and unit tests for Authors::handleArgument #3

Merged
merged 7 commits into from
Feb 18, 2020
Merged

Added branch coverage tool and unit tests for Authors::handleArgument #3

merged 7 commits into from
Feb 18, 2020

Conversation

yiiju
Copy link
Collaborator

@yiiju yiiju commented Feb 15, 2020 •
edited
Loading

Added

  • Branch coverage tool for Authors::handleArgument.
  • Add 4 new unit tests that increase the branch coverage from 58% to 73% using our coverage tool.
  • Add refactored Authors::handleArgument function into a new file called AuthorsRefactor and unit tests file called AuthorsRefactorTest to reduce cyclomatic complexity from 36 to 7.

@yiiju yiiju changed the title Handle argument Added branch coverage tool and unit tests for Authors::handleArgument Feb 15, 2020
simonsirak
simonsirak approved these changes Feb 15, 2020
View reviewed changes
Copy link
Owner

@simonsirak simonsirak left a comment

Choose a reason for hiding this comment

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

Looks good! 👍Had a couple of questions but nothing crucial. I pushed some modifications I had made to our tool, since it had some bugs. It should still work fine though, hopefully ^^

src/test/java/org/jabref/logic/layout/format/AuthorsTest.java Outdated Show resolved Hide resolved
src/test/java/org/jabref/logic/layout/format/AuthorsTest.java Outdated Show resolved Hide resolved
simonsirak
simonsirak approved these changes Feb 15, 2020
View reviewed changes
Copy link
Owner

@simonsirak simonsirak left a comment

Choose a reason for hiding this comment

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

Looks good :)

simonsirak
simonsirak approved these changes Feb 16, 2020
View reviewed changes
Copy link
Owner

@simonsirak simonsirak left a comment

Choose a reason for hiding this comment

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

Looks good!

christinerosquist
christinerosquist approved these changes Feb 16, 2020
View reviewed changes
Copy link
Collaborator

@christinerosquist christinerosquist left a comment

Choose a reason for hiding this comment

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

Looks good! Nice job, I approve 👍

@yiiju yiiju merged commit 9330e7e into master Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christinerosquist christinerosquist christinerosquist approved these changes

@SoFoDa SoFoDa SoFoDa approved these changes

@simonsirak simonsirak simonsirak approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@yiiju @simonsirak @SoFoDa @christinerosquist