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

Remove preferences and globals from tests #2768

Merged
merged 7 commits into from
Apr 20, 2017
Merged

Remove preferences and globals from tests #2768

merged 7 commits into from
Apr 20, 2017

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Apr 19, 2017 •
edited
Loading

This PR aims to make @lenhard happy again by introducing huge architectural constraints on our tests: they are not allowed to work with the preferences class anymore. As a positive side-effect also @koppor will be happy about this as tests now can no longer accidentally change or reset your preferences.

Changes in detail:

  • Remove JabRefPreferences.getInstance or Globals.prefs by mocking away all preferences class (like LayoutFormatterPreferences)
  • Refactor KeyBindinings and JournalAbbreviations so that these classes don't operate on the JabRefPreferences class anymore. This required to hide some methods of the preferences in a new interface
    PreferencesService.
  • Create architectural tests for the tests
  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 19, 2017
LinusDietz
LinusDietz approved these changes Apr 19, 2017
View reviewed changes
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

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

Wow, that's cool progress. I have scrolled through the changes and have not found anything which I would say is of concern. Is there something I should have a closer look? otherwise I would say it can be merged.

lenhard
lenhard approved these changes Apr 20, 2017
View reviewed changes
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

Wow! For a while, I have thought about how to best handle the preferences without finding a perfect solution, but this is certainly an advance. The preferences are not completely gone from logic, but if I see it correctly, then they are nicely encapsulated now. So +1 for merging.

Unfortunately, my time is a little bit too limited for a detailed review and I just scrolled over the code. That is why I do not merge directly now myself.

@Siedlerchr
Copy link
Member

In general LGTM but please have a look at the fetcher test, there seems to be some NPEs occuring wich maybe related to this PR/the mocking

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Apr 20, 2017 •
edited
Loading

Thanks for you feedback!

Ups, I forgot to fix the fetcher tests. This is done now, except that the ISBN Chimbori fetcher seems to be totally broken (@koppor this is your baby, right?)

@Siedlerchr
Copy link
Member

I merge this now. Please open a new issue for the ISBN fetcher.

@Siedlerchr Siedlerchr merged commit cbd6844 into master Apr 20, 2017
@Siedlerchr Siedlerchr deleted the removePreferencesFromTests branch April 20, 2017 12:52
Siedlerchr added a commit that referenced this pull request Apr 23, 2017
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixFileDlgs
c3bd77f 
* upstream/master:
  Reimplement date editor in JavaFX (#2781)
  Update CONTRIBUTING.md
  Add new author
  Update Checkstyle Version
  fix some more checkstyle warnings
  fix some more checkstyle warnings
  Fix Build failure, hopefully
  Spanish translation (#2773)
  Fixes #2766 If file is not found annotations might be null
  Fix language tests
  Remove preferences and globals from tests (#2768)
  Fix Unable to create Checker
  Fix checkstyle warnings
  New checkstyle rules regarding spacing
  Reimplement owner editior in JavaFX
  Reimplement url editior in JavaFX
  Reimplement journal editior in JavaFX
  New checkstyle rules regarding spacing

# Conflicts:
#	src/main/java/org/jabref/gui/importer/actions/AppendDatabaseAction.java
#	src/main/java/org/jabref/migrations/FileLinksUpgradeWarning.java
@koppor koppor mentioned this pull request Jan 1, 2020
Merged
@koppor
Copy link
Member

koppor commented Mar 11, 2020

This refs #110 (comment).

@koppor koppor mentioned this pull request Mar 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@LinusDietz LinusDietz LinusDietz approved these changes

@lenhard lenhard lenhard approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tobiasdiez @Siedlerchr @koppor @LinusDietz @lenhard