WELD 2745 Restore proper getReference behavior from 5.1.0.Final and repair CreationalContextImpl destruction and releasing logic

[ljnelson]

This PR:

• removes the createChildCc parameter from BeanManagerImpl#getReference and
• repairs the destruction and releasing logic in CreationalContextImpl while
• still allowing DependentContextTest to pass

Reference: https://issues.redhat.com/browse/WELD-2745

[ljnelson]

(I see a failure with Weld1234Test.java; not present when I run mvn clean install on my local weld/core installation. Investigating. At first glance I'm too stupid to understand how a public class ExceptionGenerator... with no toString() override and Object as its superclass could produce a String from Object#toString() that would contain REMOVED (the source of the problem).)

[ljnelson]

I followed https://github.com/weld/core#upgrading-weld-in-wildfly and https://github.com/weld/core#running-integration-tests-and-the-tck-on-wildfly and got:

java.lang.NoClassDefFoundError: org/jboss/cdi/tck/extlib/Translator
	at deployment.InstalledLibraryWarTesteac726309f6549cbb54715e112e86a7d4d9fa42.war//org.jboss.cdi.tck.tests.deployment.packaging.installedLibrary.InstalledLibraryWarTest.testInjection(InstalledLibraryWarTest.java:68)
Caused by: java.lang.ClassNotFoundException: org.jboss.cdi.tck.extlib.Translator from [Module "deployment.InstalledLibraryWarTesteac726309f6549cbb54715e112e86a7d4d9fa42.war" from Service Module Loader]
	at deployment.InstalledLibraryWarTesteac726309f6549cbb54715e112e86a7d4d9fa42.war//org.jboss.cdi.tck.tests.deployment.packaging.installedLibrary.InstalledLibraryWarTest.testInjection(InstalledLibraryWarTest.java:68)

I'll pause and wait for instructions.

[manovotn]

I followed https://github.com/weld/core#upgrading-weld-in-wildfly and https://github.com/weld/core#running-integration-tests-and-the-tck-on-wildfly and got:

java.lang.NoClassDefFoundError: org/jboss/cdi/tck/extlib/Translator
	at deployment.InstalledLibraryWarTesteac726309f6549cbb54715e112e86a7d4d9fa42.war//org.jboss.cdi.tck.tests.deployment.packaging.installedLibrary.InstalledLibraryWarTest.testInjection(InstalledLibraryWarTest.java:68)
Caused by: java.lang.ClassNotFoundException: org.jboss.cdi.tck.extlib.Translator from [Module "deployment.InstalledLibraryWarTesteac726309f6549cbb54715e112e86a7d4d9fa42.war" from Service Module Loader]
	at deployment.InstalledLibraryWarTesteac726309f6549cbb54715e112e86a7d4d9fa42.war//org.jboss.cdi.tck.tests.deployment.packaging.installedLibrary.InstalledLibraryWarTest.testInjection(InstalledLibraryWarTest.java:68)

I'll pause and wait for instructions.

That's CDI TCK ext library - only needed for 3 (or 4?) specific TCK tests so not what's failing here.
The way you can prepare WFLY for running all tests is:

• Checkout Weld and build with mvn clean install (-DskipTests)
• `export JBOSS_HOME=/path/to/wfly //CI uses nightly build for instance but you could use last released too
• mvn clean package -Pupdate-jboss-as -Dtck -f jboss-as/pom.xml
  • This updates Weld inside given WFLY dist to what your current build is
  • -Dtck is what installs the TCK ext lib
• You can now run incontainer tests by mvn clean verify -Dincontainer - be it TCKs or internal tests

And you're right that the -Dtck param is missing in the docs!

(I see a failure with Weld1234Test.java; not present when I run mvn clean install on my local weld/core installation.

The test in question is WFLY-only (marked with @Category(Integration.class)) which if why you don't see it fail locally without server.

Note that the problem could also be in some WFLY change since CI uses latest nigthly build (although it happens rarely).
This can be verified by using latest released version of WFLY - I can do that on Mon.

[manovotn]

I'll also take a look at your proposal on Mon @ljnelson as I am not near my workstation now.
I'd also love to add some tests (runnable without container) that tests the scenario we have been discussing - after all, we got here because there was no coverage, so we want to make sure we can avoid repeating the same mistake :)

[ljnelson]

With the updated instructions all tests pass on my machine using Wildfly 28.0.1.Final on my machine using JAVA_HOME=$(/usr/libexec/java_home -v11) JBOSS_HOME=/Users/lairdnelson/Downloads/wildfly-28.0.1.Final mvn clean verify -Dincontainer -f tests-arquillian/pom.xml.

[manovotn]

The test failing here is definitely unrelated. Over the weekend dependabot created some dependency update PRs and they all fail with the same error.

That's likely a WFLY change, I'm looking into that as well.
EDIT: This was likely broken by wildfly/wildfly#16885

[manovotn]

Added a few remarks.
The WFLY issue is being discussed elsewhere and the failing test can be ignored for now - if you want to see the whole CI run, feel free to mark that test with @Ignore.

[manovotn]

I've forced push into your branch @ljnelson:

• Squashed your commits into one
• Rebased onto master which now has fix for the WFLY test failure you were seeing
• Added tests for getReference that are based on your example in the JIRA and then one more for the manual interceptor resolution

[manovotn]

I am good with this PR, although I'd like to hear Martin's opinion as well.

Thanks for deep diving into this @ljnelson :)
Once merged, I'll probably consider an SP release so this gets out as fast as possible.

[mkouba]

I'm +0 for this change.

Pros:

• compatibility with older versions of Weld
• test coverage

Cons:

• stick to the behavior that is not defined by the spec; and does not make a lot of sense IMO
• even more complex code and CC#destroyed

[manovotn]

There were some bank holiday and then a PTO, hence my silence on this PR.
I am fine with its state and hoping to get back to this later this week in order to merge it and create an SP release containing this fix as well as #2862 in it.
Thanks for your extensive research and PR @ljnelson!

Martin is right that the state isn't ideal and while I agree I would prefer not to create an extra empty parent CC in most cases, I think we might want to revisit this for some future release that's not a micro with just bug fixes.