search

sismics / docs

Lightweight document management system packed with all the features you can expect from big expensive solutions
https://teedy.io
GNU General Public License v2.0
1.93k stars 486 forks source link

Fixed non-idempotent unit tests #757

Closed kaiyaok2 closed 4 months ago

kaiyaok2 commented 4 months ago

The Problem

Some unit tests are non-idempotent, as they pass in the first run but fail in the second run in the same environment. A fix is necessary since unit tests shall be self-contained. Idempotent tests help maintain this isolation by ensuring that the state of the system under test is consistent at the beginning of each test, regardless of previous test runs. For example, fixing non-idempotent tests can help proactively avoid state pollution that results in test order dependency (which could cause problems under test selection , prioritization or parallelization).

Reproduce

Use the NIOInspector plugin that supports rerunning JUnit tests in the same environment. Use TestAppResource#testInbox as an example:

cd docs-web
mvn edu.illinois:NIOInspector:rerun -Dtest=com.sismics.docs.rest.TestAppResource#testInbox

3 Non-Idempotent Tests & Proposed Fix

TestJpa#testJpa

Reason: The test creates a user without deleting it. In the second execution, an exception will be thrown when attempting to create the user, since the username is already existing.

Error message of one of the tests in the repeated run:

java.lang.Exception: AlreadyExistingUsername
    at com.sismics.docs.core.dao.UserDao.create(UserDao.java:80)
    at com.sismics.docs.BaseTransactionalTest.createUser(BaseTransactionalTest.java:55)
    at com.sismics.docs.core.dao.jpa.TestJpa.testJpa(TestJpa.java:21)

Fix: Delete the user by username and ID before returning from the test. Commit the delete afterwards.

TestUserResource#testTotp

Reason: The test creates a totp1 user without deleting it. Therefore, a HTTP 400 Bad Request error will occur on creating the user in the second execution, since the user already exists in the database.

Error message in the repeated run:

jakarta.ws.rs.BadRequestException: HTTP 400 Bad Request
    at org.glassfish.jersey.client.JerseyInvocation.convertToException(JerseyInvocation.java:939)
    at org.glassfish.jersey.client.JerseyInvocation.translate(JerseyInvocation.java:755)
    at org.glassfish.jersey.client.JerseyInvocation.lambda$invoke$1(JerseyInvocation.java:675)
    at org.glassfish.jersey.client.JerseyInvocation.call(JerseyInvocation.java:697)
    at org.glassfish.jersey.client.JerseyInvocation.lambda$runInScope$3(JerseyInvocation.java:691)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:292)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:274)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:205)
    at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:390)
    at org.glassfish.jersey.client.JerseyInvocation.runInScope(JerseyInvocation.java:691)
    at org.glassfish.jersey.client.JerseyInvocation.invoke(JerseyInvocation.java:674)
    at org.glassfish.jersey.client.JerseyInvocation$Builder.method(JerseyInvocation.java:450)
    at org.glassfish.jersey.client.JerseyInvocation$Builder.put(JerseyInvocation.java:334)
    at com.sismics.docs.rest.util.ClientUtil.createUser(ClientUtil.java:62)
    at com.sismics.docs.rest.util.ClientUtil.createUser(ClientUtil.java:45)
    at com.sismics.docs.rest.TestUserResource.testTotp(TestUserResource.java:333)

Fix: delete the totp1 user before returning from the test.

TestAppResource#TestInbox

Reason: The test initially gets inbox configurations and asserts they're in default state, for example: https://github.com/sismics/docs/blob/afa78857f97b134964bd373be46fcc32963dc69f/docs-web/src/test/java/com/sismics/docs/rest/TestAppResource.java#L253 However, the test later changes inbox configuration, for example: https://github.com/sismics/docs/blob/afa78857f97b134964bd373be46fcc32963dc69f/docs-web/src/test/java/com/sismics/docs/rest/TestAppResource.java#L272 Notice that only GET and POST methods are defined for @Path("config_inbox"), and the POST method does not support resetting hostname to the default empty string: https://github.com/sismics/docs/blob/afa78857f97b134964bd373be46fcc32963dc69f/docs-web/src/main/java/com/sismics/docs/rest/resource/AppResource.java#L451 In other words, the POST method is inherently non-idempotent, so the following assertions shall only be made in the first execution of the test:

Assert.assertFalse(json.getBoolean("enabled"));
Assert.assertEquals("", json.getString("hostname"));
Assert.assertEquals(993, json.getJsonNumber("port").intValue());
Assert.assertEquals("", json.getString("username"));
Assert.assertEquals("", json.getString("password"));
Assert.assertEquals("INBOX", json.getString("folder"));
Assert.assertEquals("", json.getString("tag"));
Assert.assertTrue(lastSync.isNull("date"));
Assert.assertTrue(lastSync.isNull("error"));
Assert.assertEquals(0, lastSync.getJsonNumber("count").intValue());

Error message of one of the tests in the repeated run:

java.lang.AssertionError: 
    at org.junit.Assert.fail(Assert.java:87)
    at org.junit.Assert.assertTrue(Assert.java:42)
    at org.junit.Assert.assertFalse(Assert.java:65)
    at org.junit.Assert.assertFalse(Assert.java:75)
    at com.sismics.docs.rest.TestAppResource.testInbox(TestAppResource.java:252)

Fix: Add a static flag to ensure that assertions with respect to default inbox configurations are only executed in the first test execution.

Verifying this change

After the patch, running the tests repeatedly in the same environment will not lead to failures.