eclipse-ee4j / eclipselink

Eclipselink project
https://eclipse.dev/eclipselink/
Other
192 stars 161 forks source link

Random JPA WDF test failures #1360

Open rfelcman opened 2 years ago

rfelcman commented 2 years ago

There are intermittent test failures in JPA WDF. It seems, that it depends on order how these tests are executed. It happens in 3.0 branch. It happens with

org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestFlush(test-jpa-wdf).testRelationshipToNew org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference(test-jpa-wdf).testPersist org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference(test-jpa-wdf).testRemoveNonExisting org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference(test-jpa-wdf).testNegativ

see https://ci.eclipse.org/eclipselink/job/eclipselink-pull-request-verifier/job/PR-1345/5/#showFailuresLink https://ci.eclipse.org/eclipselink/job/eclipselink-pull-request-verifier/job/PR-1358/1/#showFailuresLink https://ci.eclipse.org/eclipselink/job/eclipselink-pull-request-verifier/job/PR-1404/7/#showFailuresLink

dazey3 commented 2 years ago

These appear to all be negative tests; expecting failures that didn't occur. These tests appear to expect to be run in a specific order and have skated by in the past on that? But now they are running in an unexpected order? Looking at some of them, comments appear to indicate the tests do not expect some values to be in the database


org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference.testRemoveNonExisting()

This test appears to expect the entity Employee(99) shouldn't exist in the database. Look at the name of the test: "Test Remove Non-Existing".

TestGetReference.testRemoveNonExisting:

    try {
        Employee emp = em.getReference(Employee.class, Integer.valueOf(99)); // versioning, entity does not exist
        em.remove(emp);
        em.flush();
        flop("PersistenceException not thrown as expected"); //FAILS HERE
    } catch (PersistenceException e) {
        // $JL-EXC$ expected behavior
    }

The problem is that OTHER tests in TestGetReference are using the same ID (99) value!


org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference.testPersist()

This test persists Employee(99) and then expects the persist/flush to fail. Unfortunately, the test is not documented very well on what it is testing and I don't see anything obvious in the test that implies WHY persist should throw a PersistenceException.

TestGetReference.testPersist():

Employee emp = null;
            boolean operationFailed = false;
            env.beginTransaction(em);
            try {
                emp = em.getReference(Employee.class, Integer.valueOf(99));
            } catch (EntityNotFoundException e) {
                // $JL-EXC$ expected behavior
                operationFailed = true;
            }
            env.rollbackTransactionAndClear(em);

            if (emp != null) {
                env.beginTransaction(em);
                try {
                    em.persist(emp);
                    em.flush();
                } catch (PersistenceException e) {
                    // $JL-EXC$ expected behavior
                    operationFailed = true;
                }
                env.rollbackTransactionAndClear(em);
                verify(operationFailed, "persisting a hollow entity succeeded (should fail)"); // FAIL HERE
            }

ATM, I'm not quite sure WHY testPersist() expects a failure on persist in the first place. What is a "hollow entity"?

If the test doesn't fail with an expected PersistenceException, then env.rollbackTransactionAndClear(em); should rollback the INSERT. But if that isn't working, then Employee(99) exists and other tests ends up failing because they expect the entity to not exist. It looks like the tests expect a specific order AND relies on success of previous testing or a cascade of failures occurs.

Unfortunately, I'm not quite sure WHAT these tests are even testing in the first place, so I am loath to make updates that may alter the tests and lose what the test was trying to do.

dazey3 commented 2 years ago

@rfelcman @lukasj

I dug in much deeper and I am starting to wonder if these tests are just obsolete. We seem have to have good coverage of EntityManager.getReference() in eclipselink.jpa.test testing already and some of these tests in WDF TestGetReference seem to not know what they should do. Just look at this:

private boolean isHollow(Object entity) {
        return false; // TODO move to EclipseLink
        // return entity instanceof LazilyLoadable && ((LazilyLoadable)
        // entity)._isPending();
    }

Plenty of tests are using this isHollow method to determine if they should even fail at all

    boolean shouldFail = isHollow(emp);
    ...
    if (shouldFail) {
        flop("merge of a detached hollow entity succeeded (should fail)");
    }

Another issue I see is in the specification documentation for EntityManager.getReference():

Get an instance, whose state may be lazily fetched. If the requested instance does not exist in the database, the EntityNotFoundException is thrown when the instance tate is first accessed. (The persistence provider runtime is permitted to throw the EntityNotFoundException when getReference is called.)

It seems that the tests don't know if getReference will throw the ENFE or if first access on the entity will, so tests are written in such a way to validate both. That doesn't seem good to me as behavior could change without you knowing.

What are WDF tests and can we just turn these off in favor of the coverage by org.eclipse.persistence.testing.tests.jpa.advanced.EntityManagerJUnitTestSuite? Maybe port usecases to org.eclipse.persistence.testing.tests.jpa.advanced.EntityManagerJUnitTestSuite that don't exist and then delete?

rfelcman commented 2 years ago

There are some additional test failures at: https://ci.eclipse.org/eclipselink/job/eclipselink-pull-request-verifier/job/PR-1546/5/#showFailuresLink details https://ci.eclipse.org/eclipselink/job/eclipselink-pull-request-verifier/job/PR-1546/5/testReport/junit/org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager/TestUpdateBatching(test-jpa-wdf)/testPossiblyBatchedUpdate/ https://ci.eclipse.org/eclipselink/job/eclipselink-pull-request-verifier/job/PR-1546/5/testReport/junit/org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager/TestUpdateBatching(test-jpa-wdf)/testUnbacthedUpdate/