Skip to content

Tck update#104

Open
amichair wants to merge 2 commits intoapache:masterfrom
amichair:tck-update
Open

Tck update#104
amichair wants to merge 2 commits intoapache:masterfrom
amichair:tck-update

Conversation

@amichair
Copy link
Copy Markdown
Contributor

@amichair amichair commented May 8, 2026

Update TCK tests and add them back into the build.
Includes the EventListenerBridge that adds backwards compatibility necessary for the tests (though rarely needed in practice).

@amichair amichair force-pushed the tck-update branch 2 times, most recently from 5e807a2 to c80664e Compare May 8, 2026 18:30
Comment thread itests/tck/pom.xml Outdated
<dependency>
<groupId>com.sun.xml.bind</groupId>
<artifactId>jaxb-impl</artifactId>
<version>4.0.5</version>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jaxb-core and jaxb-impl are in the same version - should be in a single property?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread itests/tck/pom.xml Outdated
<groupId>org.eclipse</groupId>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>3.23.1</version>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's verify the dependency versions in this pom - dependabot does not see it, but the newer versions may be used e.g.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated all dependencies defined in this module.

Comment thread itests/tck/README.md Outdated
To run all tests:

## Run the tests
mvnw verify
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from this place it would be ../../mvnw verify?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run it from the root dir with -f... so I figured the dev would figure it out :-)
But applied your change anyway.

Comment thread itests/tck/README.md Outdated
TODO
To run a specific test:

mvnw verify -Dtck.test=<fullly.qualified.TestClass[:method]>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here ../../mvnw?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread itests/tck/pom.xml

<dependencies>

<!-- Test cases from the tck. These need to be deployed by hand as
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current approach makes it fine with the licenses?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The license in the TCK jar is Apache 2. Backed up by https://projects.eclipse.org/projects/technology.osgi.

* made that all producers within the same bundle implement the same
* interface version (or both of them).
*/
@SuppressWarnings("deprecation")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to release version 2.0.0 with the deprecation from the early beginning? when it's needed by the spec and tck then it something that will be always with us?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to explain this in the JIRA, and now added a @deprecated tag with additional explanation. In a nutshell, The EndpointListener interface itself is deprecated in the spec (i.e. should not be used by new code and will likely be removed in future spec releases), and this bridge only exists for that, so is transitively also deprecated. Users should upgrade their libraries/code to the new interface, not rely on this for the old one to work in the future. It's basically a stop-gap measure for TCK compatibility.


// bitmask of supported listener types
private static final int
EL = 1, // EndpointListener
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's name those variables ENDPOINT_LISTENER_ID and ENDPOING_EVENT_LISTENER_ID and remove comments

Copy link
Copy Markdown
Contributor Author

@amichair amichair May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used full name

private static final String
OC_PREFIX = "(" + Constants.OBJECTCLASS + "=",
OWN_LISTENER_PROP = EventListenerBridge.class.getName(),
EL_NAME = EndpointListener.class.getName(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use full names

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

EEL = 2; // EndpointEventListener

private static final String
OC_PREFIX = "(" + Constants.OBJECTCLASS + "=",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use full name OBJECT_CLASS_FILTER_PREFIX

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread itests/tck/tck.bndrun
${repo;org.glassfish.hk2.osgi-resource-locator;3.0.0}",\
org.apache.aries.rsa.discovery.tcp.address=localhost:7668,\
org.apache.aries.rsa.discovery.tcp.peers=localhost:7669,\
org.apache.aries.rsa.bridge=true,\
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we need to set a property for tck tests then does it mean that normally we are not compatible until you set this property? it should be written is some docs/readme to make users aware
TBH I would recommend changing the default to true and on demand allow users to turn off the bridge (and still have it documented)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained this at length in the JIRA - bottom line is that this is here basically just for the TCK and is extremely unlikely to ever be needed in real systems. The configuration option is left for those cases. Running it by default on all other systems that don't need it is just unnecessary runtime complexity and performance degradation (all endpoint events pass through the bridge, which also becomes a SPOF). I think it's better to leave it off (even though I spent quite some time figuring out how to do this thing with the hooks and all :-) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants