Convert remaining tests to junit 5 #5394

Siedlerchr · 2019-10-05T17:30:01Z

Remove junit4

The database test were the last remaining tests running on junit4

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Let's see if that works

matthiasgeiger

LGTM

tobiasdiez

A few comments...

tobiasdiez · 2019-10-06T08:01:20Z

src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java

+    @MethodSource("getTestingDatabaseSystems")
+    public void testMetaDataChangedEventListener(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws Exception {
+
+        bibDatabase = new BibDatabase();


The first three lines are shared between all tests, thus please extract them to the setup method.

src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java

src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java

tobiasdiez · 2019-10-06T08:13:22Z

src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java

-    public void setUp() throws SQLException, DatabaseNotSupportedException, InvalidDBMSConnectionPropertiesException {
-        this.dbmsConnection = TestConnector.getTestDBMSConnection(dbmsType);
+    @MethodSource("getTestingDatabaseSystems")
+    public void setUp(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMSProcessor dbmsProcessor) throws SQLException, DatabaseNotSupportedException, InvalidDBMSConnectionPropertiesException {


Does this really works, i.e. did you tested that all test methods are invoked correctly? According to the documentation the before/aftereach handlers don't get their parameterized resolved.

Since a test class may contain regular tests as well as parameterized tests with different parameter lists, values from argument sources are not resolved for lifecycle methods (e.g. @beforeeach) and test class constructors.

But this is in a slightly different context. If this method works, then it should also be used in DBMSSynchronizerTest.

Well I will see if I can test it locally, but otherwise Travis is green and so I thought it's working

Without double chcking (but trying to understand the implications of Tobias' explanation), I would bet that the test use only one database system and not all avaiable.

koppor · 2019-10-09T19:12:36Z

@Siedlerchr Please double check that the comments at #4260 (comment) are resolved.

Siedlerchr · 2019-10-09T19:20:38Z

The database tests are the only remaining. I checked it by removing the junit4 dependency. ArchUnit was converted a long time ago.
I will try to find time the next days to look if all tests are excuted

* upstream/master: (40 commits) Add test for regular expression pattern Snap GitHub ci (#5379) Bump src/main/resources/csl-locales from `e89e6b0` to `9785a6e` Bump src/main/resources/csl-styles from `36ac1f6` to `6169061` Bump mariadb-java-client from 2.4.4 to 2.5.0 Bump mockito-core from 3.0.0 to 3.1.0 Bump slf4j-api from 2.0.0-alpha0 to 2.0.0-alpha1 (#5403) Remove unnecessary code Fix JabFox integration Update CHANGELOG.md Add hacktoberfest shields.io badge Update CHANGELOG.md Fix various copy to clipboard issues (#5340) Fix 5359: Writing of Editor field is duplicated (#5392) Fix exception when merging entries Reenable prevcycle (#5385) Update deployment.yml revert 3fbe0a2 use submodules to fetch CSL styles properly add missing ',' ...

Siedlerchr · 2019-10-09T20:39:19Z

Ah for fucks sake, it should have been a clear warning that all tests were green before..
Turned out, they weren't executed... :(

tobiasdiez · 2019-10-21T20:33:20Z

Any hope to get this in? At least the fix that the db and fetcher tests are invoked is important.

Siedlerchr · 2019-10-21T20:36:51Z

I will try to create a woraround for it, e.g. I had the idea of "manually" calling the setup method and the clear method.

# By Tobias Diez (11) and others # Via GitHub (30) and others * upstream/master: (70 commits) Use Platform.runLater Fixes #5485 Bump com.github.ben-manes.versions from 0.26.0 to 0.27.0 Bump src/main/resources/csl-styles from `68a697b` to `c3fd4bd` Bump byte-buddy-parent from 1.10.1 to 1.10.2 Bump mariadb-java-client from 2.5.0 to 2.5.1 Bump classgraph from 4.8.48 to 4.8.52 Bump org.beryx.jlink from 2.16.0 to 2.16.2 Updated CHANGELOG.md Open entry editor by default on start-up Amend the reference to JabRefReferences initialization (#5487) Fix for issue 5463 (#5481) Rename index.md to README.md Fix not on fx thread error for custom entry types Refactor to sorted set (#5477) Removed a duplicate name (closes #5476) Remove unnecessary sort (#5470) lint CONTRIBUTING.md (#5473) Mark OOSTyle as invalid if no defaultStyle (#5471) Fix highlight problem in entry preview ... # Conflicts: # build.gradle

Siedlerchr · 2019-10-24T15:03:22Z

There are still a lot of failing tests, I guess most is related to the EntryChangedEvent stuff.

* upstream/master: New ADS Fetcher (#5501) snap: Add browser proxy for confined browser integration (#5512) Fix dark theme loading Update PreviewViewer.java [WIP] Try to fix dark theme path

call clear in setup to ensure empty tables and no leftovers from failures

Siedlerchr · 2019-10-25T16:51:44Z

For local testing it's sufficient to install mysql or postgres 10.
Only test which are failing are now the Synchronizer things. Somehow an entry is inserted without fields. I don' understand it yet.
testFieldChangedEventListener and testSynchronizeLocalDatabaseWithEntryUpdate

tobiasdiez

No idea where the problem lies...sorry.

src/main/java/org/jabref/model/entry/BibEntry.java

src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java

tobiasdiez · 2019-10-25T17:55:04Z

src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java

@@ -148,6 +156,8 @@ void testUpdateNewerEntry(DBMSType dbmsType, DBMSConnection dbmsConnection, DBMS
        bibEntry.setField(StandardField.YEAR, "1993");

        assertThrows(OfflineLockException.class, () -> dbmsProcessor.updateEntry(bibEntry));
+
+        clear(dbmsConnection);


Is it important that clear is always invoked? I think, if a assert method fails, then an exception is thrown and thus clear is never called. Either move it before assert or use try finally.

That's why I addtionally call clear now as first method in setup :-D

src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java

src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java

tobiasdiez · 2019-11-03T10:15:52Z

Any update on this one?

Siedlerchr · 2019-11-03T10:36:12Z

Did not yet find time to debug it in detail to find out where exactly the entry gets duplicated

* upstream/master: (116 commits) New translations JabRef_en.properties (French) (#5564) Select newly added jstyle in table to prevent exception (#5556) Make entry editor DND behave as specified in settings (#5554) Disabled Windows directory picker temporarily Disabled Windows directory picker temporarily Adding wix script to support jpackage update Making importing a single file easier (Issue #5508) (#5513) Fix #5551 - Don't remove unwanted characters before first author is selected (#5558) Update JabRef_it.properties New translations JabRef_en.properties (Vietnamese) New translations JabRef_en.properties (Dutch) New translations JabRef_en.properties (French) New translations JabRef_en.properties (German) New translations JabRef_en.properties (Greek) New translations JabRef_en.properties (Indonesian) New translations JabRef_en.properties (Italian) New translations JabRef_en.properties (Japanese) New translations JabRef_en.properties (Danish) New translations JabRef_en.properties (Norwegian) New translations JabRef_en.properties (Polish) ...

tobiasdiez · 2019-11-11T22:46:13Z

In order to move forward, I've added the database tests to the list of failed tests. Follow-up issue: #5602

Siedlerchr · 2019-11-12T07:38:38Z

src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java

-        assertEquals(expectedEntry.getField(StandardField.AUTHOR), actualEntries.get(0).getField(StandardField.AUTHOR));
-        assertEquals("The nano processor1", actualEntries.get(0).getField(StandardField.TITLE).get());
- //somehow the field stable is not filled
+        assertEquals(Collections.singletonList(expectedEntry), actualEntries);


This is wrong. The test explicitly checks for a changed title! Please revert!

Siedlerchr · 2019-11-12T07:39:05Z

src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java

        expectedEntry.setField(StandardField.AUTHOR, "Brad L and Gilson");
        expectedEntry.setField(StandardField.TITLE, "The micro multiplexer", EntriesEventSource.SHARED);

        List<BibEntry> actualEntries = dbmsProcessor.getSharedEntries();
-        assertEquals(1, actualEntries.size());
-        assertEquals(expectedEntry.getField(StandardField.AUTHOR), actualEntries.get(0).getField(StandardField.AUTHOR));
-        assertEquals("The nano processor1", actualEntries.get(0).getField(StandardField.TITLE).get());


See here that the title is different @tobiasdiez

Siedlerchr · 2019-11-15T07:21:07Z

build.gradle

    }

    testLogging {
        // set options for log level LIFECYCLE
-        events "failed"
+        events = ["FAILED", "STANDARD_OUT", "STANDARD_ERROR"]


@koppor just change this lines.
Im only on mobile for the next days without access to a PC 💻

Siedlerchr added 2 commits October 5, 2019 19:29

[WIP] Convert remaining tests to junit 5

ac526de

Let's see if that works

remove junit 4

9f8ad5a

Siedlerchr changed the title ~~[WIP] Convert remaining tests to junit 5~~ Convert remaining tests to junit 5 Oct 5, 2019

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 5, 2019

matthiasgeiger approved these changes Oct 6, 2019

View reviewed changes

tobiasdiez requested changes Oct 6, 2019

View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 6, 2019

koppor mentioned this pull request Oct 9, 2019

Migrate all tests to JUnit 5 #3959

Closed

Siedlerchr added 5 commits October 9, 2019 21:24

temporaily add system.out.prinltn to test if all are run

1374d20

fix gradle not executing tests correclty

9a4a52a

fix gradle syntax error

48c545c

add more logging

09b131b

fix psql driver name and url

c284667

Siedlerchr added 6 commits October 24, 2019 15:35

Manually call setup and clear in each test

0717211

fix typo and call clear

ac192c0

fix unmodifable error

a324878

fix field map error

05398b0

fix id

98f0a7c

Siedlerchr added 4 commits October 25, 2019 17:08

Merge remote-tracking branch 'upstream/master' into convertRemainingTest

6a2760c

* upstream/master: New ADS Fetcher (#5501) snap: Add browser proxy for confined browser integration (#5512) Fix dark theme loading Update PreviewViewer.java [WIP] Try to fix dark theme path

SetID does not store value as internal id field

96d65e9

add some comments for further debugging hints

b1ff631

call clear in setup to ensure empty tables and no leftovers from failures

fix copy paste error

0d380fd

tobiasdiez requested changes Oct 25, 2019

View reviewed changes

This was referenced Oct 26, 2019

Include Oracle's JDBC driver directly #5517

Merged

Refactor EntryEvents - removal part #5410

Merged

Siedlerchr changed the title ~~Convert remaining tests to junit 5~~ [WIP ] Convert remaining tests to junit 5 Nov 5, 2019

Siedlerchr mentioned this pull request Nov 5, 2019

Remove jacoco append property #5436

Merged

6 tasks

tobiasdiez added 9 commits November 11, 2019 21:30

Merge branch 'master' into convertRemainingTest

e475748

Cleanup code

06d9ec1

Don't allow codecov to fail

ae96327

Temporarily allow failure of database tests

dfb7a18

Readd database test to codecov

7152a3b

Try to ignore failures the other way

7bbd564

Or maybe this way?

b627365

Yet another try

8a82ef9

Well...maybe now

68e51e1

tobiasdiez merged commit 04015b0 into master Nov 11, 2019

tobiasdiez deleted the convertRemainingTest branch November 11, 2019 22:47

koppor changed the title ~~[WIP ] Convert remaining tests to junit 5~~ Convert remaining tests to junit 5 Nov 11, 2019

Siedlerchr commented Nov 12, 2019

View reviewed changes

koppor mentioned this pull request Nov 15, 2019

Fix expected result #5612

Merged

Siedlerchr commented Nov 15, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert remaining tests to junit 5 #5394

Convert remaining tests to junit 5 #5394

Siedlerchr commented Oct 5, 2019 •

edited

Loading

matthiasgeiger left a comment

tobiasdiez left a comment

tobiasdiez Oct 6, 2019

tobiasdiez Oct 6, 2019

Siedlerchr Oct 6, 2019

koppor Oct 9, 2019

koppor commented Oct 9, 2019

Siedlerchr commented Oct 9, 2019

Siedlerchr commented Oct 9, 2019

tobiasdiez commented Oct 21, 2019

Siedlerchr commented Oct 21, 2019

Siedlerchr commented Oct 24, 2019 •

edited

Loading

Siedlerchr commented Oct 25, 2019

tobiasdiez left a comment

tobiasdiez Oct 25, 2019

Siedlerchr Oct 25, 2019

tobiasdiez commented Nov 3, 2019

Siedlerchr commented Nov 3, 2019

tobiasdiez commented Nov 11, 2019

Siedlerchr Nov 12, 2019 •

edited

Loading

Siedlerchr Nov 12, 2019

Siedlerchr Nov 15, 2019

Convert remaining tests to junit 5 #5394

Convert remaining tests to junit 5 #5394

Conversation

Siedlerchr commented Oct 5, 2019 • edited Loading

matthiasgeiger left a comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Oct 9, 2019

Siedlerchr commented Oct 9, 2019

Siedlerchr commented Oct 9, 2019

tobiasdiez commented Oct 21, 2019

Siedlerchr commented Oct 21, 2019

Siedlerchr commented Oct 24, 2019 • edited Loading

Siedlerchr commented Oct 25, 2019

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Nov 3, 2019

Siedlerchr commented Nov 3, 2019

tobiasdiez commented Nov 11, 2019

Siedlerchr Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Oct 5, 2019 •

edited

Loading

Siedlerchr commented Oct 24, 2019 •

edited

Loading

Siedlerchr Nov 12, 2019 •

edited

Loading