12:56 a.m.
I'm seeing the following failures using hibernate.test.new_metadata_mappings=true that
appear to be regressions:
- org.hibernate.jpa.test.connection.DataSourceInjectionTest fails (also fails using the
old metadata);
- org.hibernate.test.annotations.lob.LobTest.testSerializableToBlob
-
org.hibernate.test.naturalid.mutable.cached.CachedMutableNaturalIdTest.testChangedNaturalIdNotCachedAfterTransactionFailure
Strong and Hardy, do you have an ideas about any of these?
Also, there were numerous failures in hibernate-core when building with
hibernate.test.validatefailureexpected=true, which validates that those tests annotated
with @FailureExpected or @FailureExpectedWithNewMetamodel actually fail. If an annotated
test does not fail as expected,
org.hibernate.testing.junit4.FailureExpectedHandler$FailureExpectedTestPassedException is
thrown causing the test to fail.
In most cases this is happening because there is some new functionality working now (e.g.,
sequences and other persistent ID generators) that was not working when John added
@FailureExpectedWithNewMetamodel. The fix in these cases is to simply remove
@FailureExpectedWithNewMetamodel.
When hibernate.test.validatefailureexpected is false (the default), the result from
FailureExpected tests is simply ignored.
I've gone ahead and pushed a fix that removes @FailureExpectedWithNewMetamodel from
those tests that succeed without the annotation.
There are 2 tests where this does not work:
org.hibernate.test.annotations.interfaces.InterfacesTest
org.hibernate.test.annotations.idmanytoone.alphabetical.AlphabeticalIdManyToOneTest
These tests fail with org.hibernate.cfg.NotYetImplementedException when the sources are
being processed. For some reason. this failure is not recognized as an expected failure.
I'm not sure of the reason for this.
It seems that now would be a good time to make both
hibernate.test.new_metadata_mappings=true and hibernate.test.validatefailureexpected=true
the default when building. That way we'll know when to remove
@FailureExpectedWithNewMetamodel from tests that succeed due to added functionality. It
will also help us know when there are regressions.
WDYT?
Regards,
Gail
1:26 a.m.
On 2 Aug 2012, at 07:56, Gail Badner <gbadner(a)redhat.com> wrote:
Don't know anything about the others, but LobTest is no regression. The tests did now
work at all before, because building the sources would fail. This works now and now the
test fails on the last assert. Some behavior in the engine must have changed. We can
annotate this particular test with @FailureExpectedWithNewMetamodel or maybe you know what
the problem is directly.
That flag should be the default. Why is it not turned on? In which case does it make sense
to have this flag set to false?
When hibernate.test.validatefailureexpected is false (the default),
the result from FailureExpected tests is simply ignored.
:-(
Probably because it happens I'm the setup phase and not during the test execution.
It seems that now would be a good time to make both
hibernate.test.new_metadata_mappings=true and hibernate.test.validatefailureexpected=true
the default when building.
+1
--hardy
9:10 a.m.
On Aug 2, 2012, at 1:26 AM, Hardy Ferentschik wrote:
Hardy, I'm assuming from your frown, you don't like this for some reason. This is
the expected behavior, and the main reason for having these annotations in the first
place.
That shouldn't be the case. These annotations are honored even during setup.
JPAV
9:34 a.m.
On 2 Jan 2012, at 4:10 PM, John Verhaeg wrote:
I am not following. Why does it make sense to have this flag set to false? The reason for
the annotations (@FailureExpectedWithNewMetamodel and @FailureExpected) is to mark a test
as expected to be failing.
This means when I run the tests the annotated tests should throw an exception which in
turn gets ignored as an overall test failure (the build completes nicely).
AFAIU, hibernate.test.validatefailureexpected is there to take it to the next level. What
happens if a test is annotated with a failure expected annotation and the failure does not
happen. This would often indicate
that something directly or indirectly fixed the problem with the test in which case I
would like to know about it by a failing test (the test was marked as failing, but it
actually succeeded => failure).
How are we going to gradually remove the @FailureExpectedWithNewMetamodel annotations if
there is no feedback when the tests are not failing anymore?
So unless I misunderstand the intend of hibernate.test.validatefailureexpected I think
Gail is right and it should be the default. Also the new metamodel should be the default
on the metamodel branch with the
build completing successfully. This means all failing tests should be annotated w/
@FailureExpectedWithNewMetamodel. As we implement the missing functionality and fix bugs
we keep removing them until
they are all gone. Now, this is what I thought @FailureExpectedWithNewMetamodel was all
about.
--Hardy
10:02 a.m.
On Aug 2, 2012, at 9:34 AM, Hardy Ferentschik wrote:
Ah, well that explains it. My assumption about the reason for your frown was wrong. I
thought it was the "ignoring" part you were taking issue with, thinking maybe
you were wanting some "special" exception thrown instead. I agree the validate
flag should be true by default.
Yep, we're all in agreement here. Sorry for the confusion.
JPAV
7:39 p.m.
----- Original Message -----
OK, I'll annotate org.hibernate.test.annotations.lob.LobTest with
@FailureExpectedWithNewMetamodel.
IIRC, there were some tests that were annotated with @FailureExpected that were only
expected to fail using some dialects. This was causing what looked like failures on the
dialects where the test was expected to pass. When we were trying to clean up the test
failures for the tested dialects, either hibernate.test.validatefailureexpected was
introduced or the default for hibernate.test.validatefailureexpected was changed to false.
Steve might remember exactly why this was done.
In any case, this is hindering more than helping when we're trying to get things
working with the new metamodel.
Is there a way to shift the failure so it happens during test execution so it would be
handled properly?
Agreed. I'll look into making these changes.
Thanks,
Gail
--hardy
7:44 p.m.
Hardy,
I'm seeing some more failures today using -Dhibernate.test.new_metadata_mappings=true
-Dhibernate.test.validatefailureexpected=true:
- org.hibernate.test.annotations.lob.VersionedLobTest FAILED
- org.hibernate.test.annotations.genericsinheritance.GenericsInheritanceTest FAILED
- org.hibernate.test.criteria.ComplexJoinAliasTest FAILED
Are these expected to fail as well (rather than a regression)?
Thanks,
Gail
----- Original Message -----
1:54 a.m.
Hi,
I could not tell you. Here is what I do.
1. Update the code
2. Make sure it builds (in the default settings)
3. Fix my stuff and add tests
4. Rebase
5. Make sure build works
6. Push
AFAICT there were no test failures when I checked in yesterday. I did not and will not
keep lists of failing tests using the metamodel flag.
--hardy
3:14 a.m.
----- Original Message -----
I've pushed a commit that takes care of these and changes the default for both flags
to true.
VersionedLobTest.testVersionUnchangedByteArray is marked as @FailureExpected but it
incorrectly passes using the new metamodel, so you'll see this test failing now due to
FailureExpectedHandler$FailureExpectedTestPassedException.
In addition, these tests fail:
Test org.hibernate.test.annotations.interfaces.InterfacesTest FAILED
Test org.hibernate.test.annotations.idmanytoone.alphabetical.AlphabeticalIdManyToOneTest
FAILED
Test org.hibernate.test.naturalid.mutable.cached.CachedMutableNaturalIdTest FAILED
6:39 a.m.
On 3 Jan 2012, at 10:14 AM, Gail Badner wrote
I've pushed a commit that takes care of these and changes the
default for both flags to true.
Cool, even though I would have expected that the build completes after the push. Anyways,
...
VersionedLobTest.testVersionUnchangedByteArray is marked as
@FailureExpected but it incorrectly passes using the new metamodel, so you'll see this
test failing now due to FailureExpectedHandler$FailureExpectedTestPassedException.
This one beats me. What do you mean it passes incorrectly? Switching to the new metamodel
the test passes which would indicate to me
that this test was fixed as a side effect of all metamodel work. If you are saying it
passes wrongly, then the test itself must be based on wrong
assertions. What is it?
As I said before, the problem is that @FailureExpectedWithNewMetamodel is not honored by
BaseCoreFunctionalTestCase when
an error occurs in the setup (BeforeClass handlers). We could try to address this in
BaseCoreFunctionalTestCase, but in this case I decided
to move BaseAnnotationBindingTestCase into the testing module and make it the base class
for these tests. I was using this base class for
testing some annotation based binding tests and it is imo much more suitable for the tests
above.
Right now we have the hibernate-core module passing with the new metamodel enabled and the
expected failures validated. Hopefully we can
keep it this way.
--Hardy
4:06 p.m.
----- Original Message -----
I am not convinced that the metamodel work fixes this. I could be wrong, but AFAIK,
HHH-5811 is still an issue. I can see where the assertions can be indeterminate. I've
pushed a commit to make them determinate. It still passes with the new model, but fails
with the old metadata (with -Dhibernate.test.new_metadata_mappings=false
-Dhibernate.test.validatefailureexpected=true).
My preference would be to keep @FailureExpected on this test, but I know this starts to
complicate when we add/remove annotations.
Hopefully, if it really should fail, we'll find out soon when more work is done.
Thanks for addressing the tests using annotations.
I chatted with Strong about
CachedMutableNaturalIdTest.testChangedNaturalIdNotCachedAfterTransactionFailure and it
really does look like a regression. It should work with the new metamodel so, IMO, it
should not be annotated with @FailureExpectedWithNewMetamodel. He said he would look into
it.
Gail
--Hardy
4:21 p.m.
On Aug 3, 2012, at 6:39 AM, Hardy Ferentschik wrote:
Again, not sure why you're saying this. The annotation is indeed honored in the
setup. That's where most of the errors where occurring before the annotation was
applied.
Until we finish work on the metamodel, it's still going to be important to run the
tests twice, once with the metamodel flag on, once off, and check in only if there are no
errors for either run.
JPAV
4:35 p.m.
Now I'm seeing the following failures with the old metadata (using
hibernate.test.new_metadata_mappings=false):
Test org.hibernate.test.annotations.interfaces.InterfacesTest FAILED
Test org.hibernate.test.annotations.lob.VersionedLobTest FAILED
Test org.hibernate.test.annotations.idmanytoone.alphabetical.AlphabeticalIdManyToOneTest
FAILED
It looks like BaseAnnotationBindingTestCase is hard-coded to use the new metamodel, since
it fails for the new metamodel, it still fails with
hibernate.test.new_metadata_mappings=false.
----- Original Message -----
7:32 p.m.
On 3 Jan 2012, at 11:35 PM, Gail Badner wrote:
That's right, the test will always use the new metamodel now. I think this is the way
forward for these tests in particular. The new setup is imo much more suited for these
tests and at least now there
are assertions (before there were not even assertions. the test basically was
"successful" when the setup worked)
I see myself writing more of these type of tests. I think the problem is that CustomRunner
only processes the FailureExpectedWithNewMetamodel depending on the
USE_NEW_METADATA_MAPPINGS flag.
Instead of the flag I think it should actually check which type of configuration is/was
used and process accordingly.
--Hardy
7:09 p.m.
On 3 Jan 2012, at 11:21 PM, John Verhaeg wrote:
It is NOT! Not for the org.hibernate.testing.junit4.BeforeClassCallbackHandler:
public void evaluate() throws Throwable {
try {
runner.getTestClassMetadata().performBeforeClassCallbacks( runner.getTestInstance() );
}
catch (CallbackException e) {
// be nice to see the exception. but junit seems to be eating it...
log.error( "Before class callback error : " + e.getLocalizedMessage(), e );
}
wrappedStatement.evaluate();
}
the code in question throws a CallbackException which should gets logged and things move
on. Take a debugger and step through it if you don't believe me.
-1 I don't think so. What do the others think?
--Hardy
4:17 p.m.
On Aug 3, 2012, at 1:54 AM, Hardy Ferentschik wrote:
Not sure what you mean about keeping lists. Now that we have all the annotations in
place, the only expectation is to run the build once using the default settings (which are
now with both flags set to true) and once more with the new metadata flag turned off. If
both pass w/o errors, check in. No lists required.
JPAV
7:02 p.m.
On 3 Jan 2012, at 11:17 PM, John Verhaeg wrote:
Do we really need to run both? It takes ages to run all tests. It is not easier to tag a
stable version using the old configuration and then work with two checkouts side by side.
IMO this would also allow us to be a little more aggressive with some changes. How long is
it really feasible to have both code path working at the same time w/o keeping a lot of
legacy code around. Personally I prefer to have a working reference checkout.
--Hardy
9:02 a.m.
On 08/02/2012 07:39 PM, Gail Badner wrote:
Like I said in my initial response that flag has always been there. The
reason had more to do with different audiences running the tests IIRC .
But like I said, it was years ago, so the exact specifics escape me.
I'll search the dev list when i get a few minutes and see what I can dig up.
I think you are perhaps getting caught up in making changes for a
transitory set of conditions. We currently have a slew of failures
mostly due to the tests using code they were never designed to test.
And even worse, that code is known to be incomplete. The way "failure
expected" is supposed to work is that tests which are marked as
@FailureExpected are supposed to be handled the same as if they were
marked with @Ignore (standard JUnit annotation) *if*
`hibernate.test.validatefailureexpected` is not set. If
`hibernate.test.validatefailureexpected` is set that changes the logic
up a bit. However, again the problem is that we are leveraging tests to
test something they were never designed to test. In fact, they are so
much not testing it that the code (now) under test occurs before the
test really even begins. There is supposed to be some code that skips
before/after callbacks if all tests methods are being ignored. It would
seem that code is no longer working (or @FailureExpectedWithNewMetamodel
was not hooked into that code completely) if the tests are marked for
expected failure but before callbacks still happen.
Am I against changing `hibernate.test.validatefailureexpected` to
default to true *for this branch, for the time being*? No. I already
said I would be willing to discuss that. But I'd also prefer to figure
out why what is supposed to be happening in the hibernate-testing code
is no longer happening.
When hibernate.test.validatefailureexpected is false (the default),
the result from FailureExpected tests is simply ignored.
> :-(
Not understanding how this is not desirable?
Is there a way to shift the failure so it happens during test execution so it would be
handled properly?
Sure. You would not use @Before, @BeforeClass, @BeforeClassOnce
callbacks. Instead you would build the metamodel during the test method
execution. But again, see above about tests testing code they were not
designed to test...
--
steve(a)hibernate.org
http://hibernate.org
1:01 a.m.
----- Original Message -----
I'm not sure I understand what you mean? Do you mean that the FailureExpected code is
not intended to expect failures in @Before, @BeforeClass, and @BeforeClassOnce callbacks?
Yes, this should be fixed.
+1
If the result is simply ignored, then we don't know if changes would result in a
@FailureExpectedWithNewMetamodel test to actually pass.
With the validation flag set to true, a failure due to FailureExpectedTestPassedException
would (hopefully) mean that the @FailureExpectedWithNewMetamodel annotation can be
removed. Once it's removed, we have a way to see if a future change breaks it.
I agree that this is not the way to fix this.
Thanks,
Gail
8:02 a.m.
On Tue 07 Aug 2012 01:01:37 AM CDT, Gail Badner wrote:
That is exactly what I am saying. You have to step back and look at
how tests are structured. And this is not specific to Hibernate tests
*in any way*. Test generally have (1) a set-up phase (2) a test phase
and (3) a tear-down phase. set-up and tear-down are meant to deal with
things called test *fixtures*. Google shows all kinds of great hits on
definitions for test fixtures so i wont go into details. But the gist
is that building a test fixture is not at all expected to fail; its not
the "thing under test". This is the exactly what we have discussed a
few times now. If you are trying to "test something" the only way to
do that reasonably well is to do it in that second "test phase". The
existing tests simply were not written on the premise they are now
being twisted to.
In thinking about this more, I think this situation makes some sense.
I was confused about why JUnit was just "eating" the failures. IIUC
JUnit is seeing failures in the set up phase and is simply ignoring the
test. What John and I discussed when we worked out the
@FailureExpectedWithNewMetamodel was to possibly "store" start up
failures (exceptions) and use them for each grouped test method. I
think that is not unreasonable, even long term, so I will work on that
change after 4.1.6.
Part of this goes to the other email you sent out last night; I will
respond to that separately.
--
steve(a)hibernate.org
http://hibernate.org
11:08 a.m.
On 7 Aug 2012, at 15:02, Steve Ebersole <steve(a)hibernate.org> wrote:
Exactly what I was trying to say a :-) And this is exactly why I rewrote the tests we have
been discussed in this thread. Most of them were not even doing/asserting anything when
reaching the second phase. The "test" was just reaching the second phase. Really
the tests should have been written differently and not even extend our main base class for
tests.
With the old Configuration it was admittedly hard to write other type of tests, but here
is one of the strength of the new metamodel. The building of the configuration is now much
more open for testing. Many things don't need to be tested going all the way to the db
(also good performance wise!) For this reason I envision also a whole set of different
base classes each addressing different testing needs.
In thinking about this more, I think this situation makes some sense.
I was confused about why JUnit was just "eating" the failures. IIUC JUnit is
seeing failures in the set up phase and is simply ignoring the test.
+1
Where possible I would prefer better written test, but given st we have so many legacy
test it probably makes sense to take care of this.
--hardy
11:24 a.m.
On Tue 07 Aug 2012 11:08:27 AM CDT, Hardy Ferentschik wrote:
Yep, this is a great point about re-writing. If you are testing
something, direct tests/assertions are always the best route.
I think its ok to extend the BaseUnitTestCase. But totally agree that
extending BaseCoreFunctionalTestCase does not make any sense for tests
specifically testing Metadata building (heck that is really the case
for tests that test building a Configuration too).
For this reason I envision also a whole set of
different base classes each addressing different testing needs.
Well?!? Where are they? :D J/K, but yeah, maybe we do need some new
tests specific to building a ServiceRegistry as a fixture, building a
MetadataSources as a fixture, etc...
Yep that was my thought process as well. This is pretty clearly a case
of badly written tests or tests being used in ways they were not
intended. But what I suggest should be a pretty small change and not
bad in and off itself. And would certainly help in this situation.
And we have to keep in mind that once this process of building a
SessionFactory from Metadata moves from transitory, those tests will
again be correct (even as they are now!) and useful. Its not the tests
themselves that are "bad" (at least a lot of them we discuss lately).
We just happen to be using them in a non-anticipated and arguably
non-correct way as we make this transition.
I think we just need to keep in mind that the intent here is really to
flush out what is still lacking in the metamodel code. That was the up
front discussion and the only way it makes sense to use these tests as
we are right now during this transition.
--
steve(a)hibernate.org
http://hibernate.org
5:55 p.m.
On Tue 07 Aug 2012 11:24:34 AM CDT, Steve Ebersole wrote:
Ok, this may not be possible after all. At least, it may not be
possible in conjunction with "failure expected". The problem lies in
the fact of how JUnit layers callbacks (and how we tie into that
process) into the execution of a test. It uses a contract called
Statement to represent something to be executed. At the very base you
have a Statement that performs the test method. Around that you have
another Statement that applies @Before callbacks. Around that you have
yet another Statement that applies @After callbacks, Etc. It is
essentially "around advice", each successive level delegating to the
next after applying its special advice.
The problem is that our Statement for "failure expected" handling is
"inside" the Statement for callbacks.
--
steve(a)hibernate.org
http://hibernate.org
8:24 a.m.
On 08/02/2012 12:56 AM, Gail Badner wrote:
These tests fail with org.hibernate.cfg.NotYetImplementedException
when the sources are being processed. For some reason. this failure is not recognized as
an expected failure. I'm not sure of the reason for this.
I was looking at this
a little before I left for vacation (not these
specific failures, just the general phenomenon). As far as I can tell
it is a problem in the hibernate-testing code (and perhaps a change in
JUnit between when that code was originally developed and the versions
of JUnit used now). JUnit now seems to just "eat" startup (@Before,
etc) errors and the "failure expected" handling code does not hook in to
the startup/shutdown process, its only in effect during the actual run.
I added a dump of the exception when this happened so we could at least
see the cause of the test not being run. But I never had a chance to
go back and look after John added the @FailureExpectedWithNewMetamodel
hooks. So not sure if this is still related.
+1000 that 'hibernate
.test.new_metadata_mappings' should be true by
default on this branch. Its the whole reason for this branch.
We can discuss making 'hibernate .test.validatefailureexpected' true by
default as well. The default has actually been false from the very
beginning of me developing that code. But that was years ago. I do not
remember the specific whys anymore.
--
steve(a)hibernate.org
http://hibernate.org
4621
days inactive
4626
days old
25 comments
4 participants
participants (4)
-
Gail Badner -
Hardy Ferentschik -
John Verhaeg -
Steve Ebersole