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

Refactoring and addition of unit tests #7597

Merged
merged 32 commits into from
Jun 21, 2021
Merged

Refactoring and addition of unit tests #7597

merged 32 commits into from
Jun 21, 2021

Conversation

nasdas-dev
Copy link
Contributor

This pull request includes new unit tests for three classes which increases their line/statement coverage.
They contribute to issue #6207.

Further, I have also refactored existing unit tests to make them more readable, understandable and maintanable.
In this sense, I focused more on test smells described in the paper Refactoring test code (van Deursen et al.) and also on the Code Howtos (Test Cases) in the Development Documentation.

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

FieldChange.java
18% -> 94%
Abbreviation.java
63% -> 88%
SuggestionProviders.java
0% -> 100%
FileHelper.java
-> Boundary testing of an empty file

CitationKeyGenerator.java
-> Boundary testing of testlagepage parser for 0-00 & 1-1

HTMLCharacterChecker.java
-> Null Value Boundary test
ParsedEntryLink.java
-> Partition testing of ParsingEntryLink

UpperCaseFormatter.java
-> Partition testing for special characters

CitationStyleCacheTest.java
-> Partition testing of cache storage
Checkstyle passed
Assertion Roulette
Added Resource Optimism
Added assertion messages to fix assertion roulette.
General Fixture, removed test code duplication
Fixed AssertionRoulette, one instance of duplicated test code.
Assertion Roulette fixed
fixed Assertion Roulette
@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Mar 30, 2021
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Thanks for adding to our test suite.
Nevertheless, got some remarks.

@koppor
Copy link
Member

koppor commented May 6, 2021

@nasdas-dev May I ask whether you will find time to address our comments?

@nasdas-dev
Copy link
Contributor Author

@koppor I'll have a look this weekend! Sorry for the delay

@nasdas-dev
Copy link
Contributor Author

@koppor I've applied your feedback for all files on this pull request as well as the following: #7653
Unfortunately, I've realized that I committed the same files multiple times on these PRs. Could you please give me a suggestion, how to fix this problem in the most pragmatic way? e.g. git revert?

@koppor
Copy link
Member

koppor commented May 17, 2021

We closed the other PR #7653, so no worries if the files are duplicate.

The normal way would be to do some git reset magic, but it's hard to master. I would really recommend to read https://lostechies.com/joshuaflanagan/2010/09/03/use-gitk-to-understand-git/.

Could you work on this PR and resolve the conflicts? Then, we can review and merge.

@calixtus
Copy link
Member

Hi @nasdas-dev , did you already had some time to get this branch and the corrupt commits right?

koppor added 10 commits June 18, 2021 22:47
…ngxie1991-a2-ds

# Conflicts:
#	src/test/java/org/jabref/gui/autocompleter/SuggestionProvidersTest.java
#	src/test/java/org/jabref/logic/citationkeypattern/CitationKeyGeneratorTest.java
#	src/test/java/org/jabref/logic/formatter/casechanger/UpperCaseFormatterTest.java
#	src/test/java/org/jabref/model/entry/EntryLinkListTest.java
- Remove assertion failure messages
- Fix List.of()
- Fix variable name
- convert to parameterized test
- Fix checkstyle / indent
- Revert some changes
@koppor koppor removed the status: changes required Pull requests that are not yet complete label Jun 20, 2021
@koppor
Copy link
Member

koppor commented Jun 20, 2021

@nasdas-dev I took the freedom to finalize the PR. Most importantly, I used the power of JUnit's parameterized tests.

Copy link
Member

@calixtus calixtus 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 to me

@Siedlerchr
Copy link
Member

one failing unit test
org.jabref.model.FieldChangeTest


  Test differentFieldChangeIsNotEqual() FAILED

  org.opentest4j.AssertionFailedError: expected: <FieldChange [entry=, field=DOI, oldValue=foo, newValue=bar]> but was: <FieldChange [entry=, field=DOI, oldValue=fooX, newValue=barX]>

@calixtus
Copy link
Member

calixtus commented Jun 21, 2021

Looks good to me

Way to quick again... 🙈 I'll look into it.

@koppor koppor merged commit 4e93f71 into JabRef:main Jun 21, 2021
Siedlerchr added a commit that referenced this pull request Jun 22, 2021
* upstream/main:
  New Crowdin updates (#7843)
  Refactoring and addition of unit tests (#7597)
  CLI option to write XMP metadata to pdfs (#7814)
  Add query validation for web search (#7809)
  change eclipse default output dir (#7842)
  Bump lucene-queryparser from 8.8.2 to 8.9.0 (#7835)
  Bump libreoffice from 7.1.3 to 7.1.4 (#7836)
  Bump postgresql from 42.2.21 to 42.2.22 (#7839)
  Bump org.eclipse.jgit (#7838)
  Bump byte-buddy-parent from 1.11.2 to 1.11.5 (#7837)
  Bump unoloader from 7.1.3 to 7.1.4 (#7841)
  Ms Office Export patent author as inventor (#7831)
  Abbreviation toggle within the JournalEditorViewModel now ignores curly braces (issue #7773) (#7807)
Siedlerchr added a commit that referenced this pull request Jun 30, 2021
* upstream/main: (26 commits)
  Add unit test to four test classes (#7651)
  Fix IEEE test (#7852)
  New Crowdin updates (#7859)
  Fix markdown syntax of ADRs
  add missing l10n (#7857)
  New Crowdin updates (#7847)
  Bump mockito-core from 3.11.1 to 3.11.2 (#7856)
  Bump checkstyle from 8.43 to 8.44 (#7855)
  Fix for issue #4652: Add Find Unlinked Files Filter based on Date (#7846)
  Fix for entering a backslash in the custom entry preview dialog (#7851)
  Fixed INSPIREFetcherTest
  Fixed TitleFetcherTest
  Ignore baeldung.com and tldrlegal.com from out link checks
  New Crowdin updates (#7845)
  New Crowdin updates (#7843)
  Refactoring and addition of unit tests (#7597)
  CLI option to write XMP metadata to pdfs (#7814)
  Add query validation for web search (#7809)
  change eclipse default output dir (#7842)
  Bump lucene-queryparser from 8.8.2 to 8.9.0 (#7835)
  ...
Siedlerchr added a commit that referenced this pull request Jul 4, 2021
…kflow-for-slr-search

* upstream/main: (31 commits)
  New translations JabRef_en.properties (German) (#7868)
  Fix test "higherTrustLevelWins()" (#7866)
  Change WM_CLASS to jabref (#7858)
  [Bot] Update CSL styles (#7865)
  Add unit test to four test classes (#7651)
  Fix IEEE test (#7852)
  New Crowdin updates (#7859)
  Fix markdown syntax of ADRs
  add missing l10n (#7857)
  New Crowdin updates (#7847)
  Bump mockito-core from 3.11.1 to 3.11.2 (#7856)
  Bump checkstyle from 8.43 to 8.44 (#7855)
  Fix for issue #4652: Add Find Unlinked Files Filter based on Date (#7846)
  Fix for entering a backslash in the custom entry preview dialog (#7851)
  Fixed INSPIREFetcherTest
  Fixed TitleFetcherTest
  Ignore baeldung.com and tldrlegal.com from out link checks
  New Crowdin updates (#7845)
  New Crowdin updates (#7843)
  Refactoring and addition of unit tests (#7597)
  ...

# Conflicts:
#	src/main/resources/l10n/JabRef_en.properties
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.

5 participants