Skip to content

Add Tests to DoiCleanup #8124

Merged
koppor merged 12 commits into
JabRef:mainfrom
brunocmo:main
Oct 7, 2021
Merged

Add Tests to DoiCleanup #8124
koppor merged 12 commits into
JabRef:mainfrom
brunocmo:main

Conversation

@brunocmo

@brunocmo brunocmo commented Oct 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adding unit tests to DoiCleanup class, to improve the coverage of the project.

I added 7 test to the file:

  1. cleanupDoientryJustdoi
  2. cleanupDoiEntryJustDoiAllEntries
  3. cleanupDoiEntryDoiFieldsWithoutHttp
  4. cleanupDoiEntryDoiWithSpaces
  5. cleanupDoiJustUrl
  6. cleanupDoiJustNote
  7. cleanupDoiJustEe
  • 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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

koppor
koppor requested changes Oct 6, 2021
View reviewed changes

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for working on tests. Since this also seems to be a coding exercise, some code feedbacks:

Besides small comments, I would suggest to convert all tests checking to a ParameterizedTest. See org.jabref.logic.bst.BibtexCaseChangersTest#testChangeCaseAllLowers for an example. Can be written even more simple in your case --> first paramter the expected entry, the second one the BibEntry to cleanup.

Comment thread src/test/java/org/jabref/logic/cleanup/DoiCleanupTest.java Outdated
Comment thread src/test/java/org/jabref/logic/cleanup/DoiCleanupTest.java Outdated
Comment thread src/test/java/org/jabref/logic/cleanup/DoiCleanupTest.java Outdated
Comment thread src/test/java/org/jabref/logic/cleanup/DoiCleanupTest.java Outdated
@brunocmo

brunocmo commented Oct 7, 2021

Copy link
Copy Markdown
Contributor Author

Thank you for working on tests. Since this also seems to be a coding exercise, some code feedbacks:

Besides small comments, I would suggest to convert all tests checking to a ParameterizedTest. See org.jabref.logic.bst.BibtexCaseChangersTest#testChangeCaseAllLowers for an example. Can be written even more simple in your case --> first paramter the expected entry, the second one the BibEntry to cleanup.

Thanks for the feedback and tips. I took your sugestion and changed to ParameterizedTest. Still I don't know if the implementation is correctly, if you can spot anything wrong again I would be thankfully.

@brunocmo brunocmo requested a review from koppor October 7, 2021 03:26
@koppor koppor merged commit dd5faee into JabRef:main Oct 7, 2021
@koppor koppor mentioned this pull request Oct 9, 2021
Closed
2 tasks
Siedlerchr added a commit that referenced this pull request Oct 10, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into jabrefonline
0b9f414 
* upstream/main: (149 commits)
  Add Tutorials for javafx
  lint
  Add changelot
  Open folder on mac and highlight file
  Add Tests to DoiCleanup (#8124)
  Improve Drag and Drop in Custom Entry types dialog (#8121)
  Show preview also for available styles (#8110)
  udpate to javafx 17.0.0.1
  Don't throw exception when validating invalid paths (#8112)
  Bump jackson-dataformat-yaml from 2.12.5 to 2.13.0
  Bump jackson-datatype-jsr310 from 2.12.5 to 2.13.0
  Bump byte-buddy-parent from 1.11.15 to 1.11.18
  Bump classgraph from 4.8.116 to 4.8.121
  Bump checkstyle from 9.0 to 9.0.1
  remove iso charset, website returns utf8 for icar comp sci only returns one result
  fix springer fetcher Fix computer science fetcher
  Squashed 'buildres/csl/csl-locales/' changes from 7a507fc008..495f888637
  Squashed 'buildres/csl/csl-styles/' changes from 5facb37..3b00357
  Update CHANGELOG.md
  snap: Use lzo compression & switch to core20 base
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

2 participants

@brunocmo @koppor