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

Test context framework: flush problems should lead to test exceptions instead of failures [SPR-5699] #10369

Closed
spring-projects-issues opened this issue Apr 26, 2009 · 13 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Apr 26, 2009

Marek Kaluzny opened SPR-5699 and commented

Since improvement made for #9988 it is impossible to write a test method with expected exception. For example I've got test which triggers unique constraint violation exception and that violation throw JpaSystemException. Marking test with @Test(expected) or @ExpectedException doesn't work, because test always ends with failure since
org.springframework.test.context.transaction.TransactionalTestExecutionListener.endTransaction
called by
org.springframework.test.context.junit4.SpringMethodRoadie.runAfters
will always try to flush the transaction.

I think that exceptions threw at the end of transaction should be propagated as test execeptions not as failures.
I suppose that that was the main idea of ticket #9988.


Affects: 3.0 M2

Issue Links:

  • #9988 Test context framework: @Transactional does not flush the Hibernate session before rollback
  • #10613 Document potential false positives when testing ORM code

Referenced from: commits ee1938e, 2932779

4 votes, 9 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 18, 2009

Jussi Nieminen commented

Also there is another problem if using Hibernate. I was trying to also test that unique constain work in my Hibernate code. What happens is that when saving hibernate object that violates unique constraint Hibernate throws error as expected. Still the object stays in Hibernate session but never got Id generated and thus Id is null. Now endTransaction method tries to flush the HIbernate session which will end up throwing exception "org.hibernate.AssertionFailure: null id in .persistence.dto.User entry (don't flush the Session after an exception occurs)".

I'm using 3.0M3

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2009

Trond Marius Øvstetun commented

A simple workaround for the Hibernate-issue is to clear your session in your test-method.

try {
em.persist(failinEntity);
fail();
catch (DataIntegrityViolationException e) {
em.clear();
}

This will remove your object from the hibernate session. Not an ideal solution, this issue needs to be fixed.
Is the transaction committed even with an exception thrown from the test-method? This is scary stuff, will the same be an issue in production-code as well?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 11, 2009

Sam Brannen commented

Hi guys,

I've done a bit of investigating on this issue and #9988, and I've come to the following conclusion:

The changes introduced in TransactionalTestExecutionListener for SPR-5315 should be removed, and developers should manually flush the underlying session within each test method.

This may not sound very ideal, but here is my reasoning:

The afterTestMethod() method of any TestExecutionListener (specifically TransactionalTestExecutionListener in this case) is called after the test method itself has completed execution as well as after all @After methods of the testing framework (e.g., JUnit).

The testing framework (e.g., JUnit 4.6) wraps the call to the test method in a try-catch block, and if the test method throws an exception, the testing framework checks if the exception was expected. Therefore, an expected exception must be thrown by the test method itself, not by a before or after method. This also applies to TransactionalTestExecutionListener: if a call to transactionStatus.flush() within the TransactionalTestExecutionListener throws an exception, that exception will not be processed as an expected exception.

As stated in #9988, failing to flush the ORM tool's underlying session can lead to false positives in test cases; however, as pointed out in this issue, flushing the session in TransactionalTestExecutionListener makes it impossible for the testing framework to process the exception (which may have resulted from flushing the session) as an expected exception.

I prototyped adding logic to TransactionalTestExecutionListener to prevent the session from being flushed when an exception was thrown during the test method; however, choosing not to flush the session in such scenarios is only possible if we change the semantics of the underlying testing framework (e.g., JUnit) with regard to expected exceptions and their scope (i.e., by changing the scope to include: before methods, the test method itself, and after methods). Such a drastic change is likely not even possible with TestNG, which makes it actually a non-option.

So in summary, we have to make a trade off between (1) automatic flushing of the ORM tool's session and (2) support for expected exceptions, and in my opinion the latter should clearly win.

Having said that, I'm naturally open to discussion. ;)

Regards,

Sam

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 11, 2009

Sam Brannen commented

FYI: if you're interested in reviewing the test case I created for this issue to see if it falls in line with your own code, either click on the FishEye tab above or check out HibernateSessionFlushingTests and related files here:

https://fisheye.springsource.org/browse/spring-framework/trunk/org.springframework.test/src/test/java/org/springframework/test/context/junit4/orm/HibernateSessionFlushingTests.java?r=HEAD

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 12, 2009

Oliver Drotbohm commented

Second your decision, Sam. Maybe should file a ticket for JUnit to allow TestExecutionListener Exceptions to be checked for the a possible annotated expected Exception type?

Regards,
Ollie

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 15, 2009

Sam Brannen commented

Hi Ollie,

Maybe should file a ticket for JUnit to allow TestExecutionListener Exceptions to be checked for the a possible annotated expected Exception type?

That's unfortunately not be feasible. Doing so would (1) change the semantics of JUnit and (2) simultaneously break code which expects exceptions in TestNG based tests. This discrepancy is exactly the reason I consider it a non-option: semantics of the Spring TestContext Framework must remain consistent across testing frameworks (e.g., JUnit and TestNG).

Regards,

Sam

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Trond Marius Øvstetun commented

I agree that you should drop automatic flushing and so allow support for expected exceptions.
I have used this approach a long time, and found the new semantics suddenly broke a whole lot of my old tests.. My tests use a manual flush to provoke the error I want. Whether I catch or not in my tests now, the test will fail after my test-method has finished.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Sam Brannen commented

Thanks for your input guys.

Juergen and I are discussing it.

  • Sam
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Grzegorz Borkowski commented

I had also the problem related to this issue today. The strange thing is, what also Trond mentioned: it seems that after exception is thrown, the transaction gets committed.
In my case I have several tests, in setup() method I insert one record which is used in all tests. After each test transaction is rollbacked, so the row is removed. However, one test tries to add the second record violating unique constraint on one column. Then hibernate throws "AssertionFailure: null id" (as in Jussi's case) ; but after this, all other tests fail at setup method with message that row can't be insterted because it exists. So it looks like the transaction is committed after the exception is thrown - but it shouldn't.

For the general discussion: I also agree that @ExpectException is more important than automatic flushing. But obviously, automatic flushing is also logical in tests context, it's said it cannot be integrated... (just wonder, without analysing the whole issue: JUnit 4 has build-in "expect exception" capability on @Test annotation, I guess TestNG must have such thing too... we dont' probably care any longer about junit 3.x, so perhaps the Spring's @ExpectException can be discarded at all, and native test framework support used instead - perhaps it can help this problem?)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Sam Brannen commented

Grzegorz, thanks for your input.

FYI: the discussion is actually not about @ExpectedException but rather about expected exceptions in general. The issue being discussed here is independent of whether you use @ExpectedException or @Test(expected=...) (in the case of JUnit 4).

As an aside, we recommend that you use @ExpectedException only with JUnit 3.8. Furthermore, we generally recommend that you upgrade to JUnit 4.6 or TestNG 5.9.

With regard to relying on JUnit 4's and TestNG's built-in support for expected exceptions, no, that would not help here. See my above comments regarding expected exceptions and their scope.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Grzegorz Borkowski commented

Thanks Sam, I read your description of the problem once again and now I see that in fact @ExpectException is even better suited to solve the problem, because it is tested (I assume) outside the junit's try-catch block, so possibly it can be tested after transaction listener, and after flush() is called. But in such a case developer's would be forced to use @ExpectException instead of native @Test(expected=...), which seems ugly to me. From what I understand the only solution that could work with @Test(expected=) is wrapping each test method call by using some aspect/proxy of test class. However I'm not sure if junit/testng architecture allows for it. Besides, it's stupid to think that Spring guys haven't checked such option yet :)
BTW: is junit 3.8 going to be supported in Spring 3.0? I guess that if anybody uses 3.8 instead of 4.x, then it is because they use java 1.4. Spring 3.0 requires java 5, so it seems logical that everybody who uses Spring 3.0 will also use JUnit 4...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Sam Brannen commented

Reverted changes to TransactionalTestExecutionListener: transactions are no longer automatically flushed before rolling back.

As a side effect, support for expected exceptions once again works as expected. For a working example, consult the updated HibernateSessionFlushingTests class mentioned earlier in the discussion.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 20, 2009

Sam Brannen commented

BTW: is junit 3.8 going to be supported in Spring 3.0? I guess that if anybody uses 3.8 instead of 4.x, then it is because they use java 1.4. Spring 3.0 requires java 5, so it seems logical that everybody who uses Spring 3.0 will also use JUnit 4...

The legacy JUnit 3.8 class hierarchy (i.e., AbstractDependencyInjectionSpringContextTests and friends) is deprecated in Spring 3.0; however, JUnit 3.8 in the Spring TestContext Framework is still supported where possible, but only as a migration path from the legacy code to the TestContext framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants