jakartaee / validation-tck

Jakarta Validation TCK
http://validator.hibernate.org
Apache License 2.0
38 stars 33 forks source link

BVTCK-188 Allow bootstrap exceptions to be thrown at deployment #152

Closed gsmet closed 6 years ago

gsmet commented 6 years ago

https://hibernate.atlassian.net/browse/BVTCK-188 https://hibernate.atlassian.net/browse/BVTCK-190 https://hibernate.atlassian.net/browse/BVTCK-191

aguibert commented 6 years ago

@gsmet here is the full set of classes I found that had this issue:

org.hibernate.beanvalidation.tck.tests.bootstrap.BootstrapConfigurationWithEmptyValidatedExecutableTypesTest
org.hibernate.beanvalidation.tck.tests.bootstrap.customprovider.BootstrapNonAvailableValidationProviderTest
org.hibernate.beanvalidation.tck.tests.valueextraction.declaration.MultipleValueExtractorsInValidationXmlForSameTypeAndTypeArgumentTest
org.hibernate.beanvalidation.tck.tests.valueextraction.declaration.ValueExtractorWithNoPublicNoArgConstructorInValidationXmlTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.ClockProviderSpecifiedInValidationXmlNoDefaultConstructorTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constraintdeclaration.ConfiguredBeanNotInClassPathTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constraintdeclaration.fieldlevel.WrongFieldNameTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constraintdeclaration.MandatoryNameAttributeTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constraintdeclaration.MissingMandatoryElementTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constraintdeclaration.propertylevel.WrongPropertyNameTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constraintdeclaration.ReservedElementNameTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.constructorvalidation.UnknownConstructorValidationTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.invalid.InvalidValidationXmlTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.MessageInterpolatorSpecifiedInValidationXmlNoDefaultConstructorTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.methodvalidation.MethodAsGetterAndMethodNodeTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.methodvalidation.UnknownMethodValidationTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.MissingClassNameOnBeanNodeTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.TraversableResolverSpecifiedInValidationXmlNoDefaultConstructorTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.versioning.UnknownVersionInValidationXmlTest

Also, I noticed a few other tests were failing for me because the test apps are missing some required classes:

// these apps need to include org.hibernate.beanvalidation.tck.tests.xmlconfiguration.versioning.DummyMessageInterpolator
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.versioning.Version10InValidationXmlTest
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.versioning.Version11InValidationXmlTest

// this app needs to include the inner class CustomMessageInterpolatorTest.DummyMessageInterpolator
org.hibernate.beanvalidation.tck.tests.bootstrap.CustomMessageInterpolatorTest

// this app needs to include org.hibernate.beanvalidation.tck.tests.xmlconfiguration.versioning.DummyClockProvider
org.hibernate.beanvalidation.tck.tests.xmlconfiguration.versioning.Version20InValidationXmlTest
aguibert commented 6 years ago

I pulled this change in locally and it seems to be working. I need to make a few additional tweaks on top of it, which I can contribute via another PR.

gsmet commented 6 years ago

@aguibert please wait for a bit. I'm working on it and will push a squashed version soon. Will let you know.

gsmet commented 6 years ago

I already updated all the tests you mentioned. I'm checking the issue you reported with the missing classes (I'm a bit surprised because we are running the TCK tests in WildFly with proper isolation but will check anyway).

gsmet commented 6 years ago

@aguibert so indeed, you're right on the XML tests but I have a hard time understanding the issue with this one:

// this app needs to include the inner class CustomMessageInterpolatorTest.DummyMessageInterpolator
org.hibernate.beanvalidation.tck.tests.bootstrap.CustomMessageInterpolatorTest

As we include the test, the inner class should be copied with the test.

gsmet commented 6 years ago

@aguibert I just force-pushed a new version with all the tests you mentioned fixed (and tested everything in WF container mode - so I consider this version OK for us).

Would appreciate if you could validate it works OK for you too.

The only issue I haven't fixed so far is this one:

// this app needs to include the inner class CustomMessageInterpolatorTest.DummyMessageInterpolator
org.hibernate.beanvalidation.tck.tests.bootstrap.CustomMessageInterpolatorTest

as I don't understand why it wouldn't work as is.

I'm finished with my changes for now so feel free to contribute additional work on top of it.

aguibert commented 6 years ago

@gsmet you're correct, I double checked CustomMessageInterpolatorTest and no change is needed. I had thought initially it wouldn't package the DummyMessageInterpolator and Person inner classes, but it does.

gsmet commented 6 years ago

@aguibert so do you pass the TCK with the version in this PR, now? Are other changes required?

Thanks for your feedback!

aguibert commented 6 years ago

@gsmet All of the TCK-side issues appear to be resolved now. Thanks! There was one minor tweak I had to make so the TCK builds on Windows, but I can contribute that separately.

I've got OpenLiberty almost passing the TCK now with your most recent set of changes from this PR. Currently 1036 tests pass. Still need to get the JavaFX stuff sorted out, and then there is one additional issue with OpenLiberty where specifying an app-defined <default-provider> in validation.xml does not work, but that'll need to be a fix I need to make in the OpenLiberty code.

gsmet commented 6 years ago

@aguibert I integrated a trimmed down version of your patch for the Windows support in this PR.

Could you please validate that everything is OK for you with this branch?

Thanks!

gsmet commented 6 years ago

@aguibert

I added a few commits to be on the safe side. I'm finished on this.

Here is what I suggest:

If you really need a new version, we could release earlier but I really think it would be better to be sure all your issues are sorted out before making a new release.

Feedback welcome!

aguibert commented 6 years ago

hi @gsmet, it will probably be at least a week before I have time to work through the remaining TCK issues with OpenLiberty. Until then I can work off of a local snapshot with these changes

gsmet commented 6 years ago

If it works for you to work with the snapshot, let's do that.

And we'll release once we are totally sure we fixed all your issues.

aguibert commented 6 years ago

hi @gsmet just wanted to confirm that this PR plus the exclusion of the JavaFX tests gets OpenLiberty 100% passing for the BeanVal 2.0 TCK. Thanks for all of your help on this!

gsmet commented 6 years ago

@aguibert by JavaFX tests exclusion, you mean the other PR? Would be nice if you could just get the branch from the other PR, build it and test the TCK (just checking, it's what you're done so that we are sure everything is OK before releasing 2.0.2.Final).