Skip to content

fix(activemq-client:6.0.0): resolve test failures #599

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erguiti-imane
Copy link
Member

What does this PR do?

  • Removed reliance on the legacy javax.jms API and use jakarta.jms instead
  • Regenerate GraalVM metadata for activemq-client 6.0.0

Checklist before merging

- Removed reliance on the legacy javax.jms API and use jakarta.jms instead
- Regenerate GraalVM metadata for activemq-client 6.0.0
@erguiti-imane erguiti-imane requested a review from a team as a code owner February 19, 2025 10:15
@erguiti-imane erguiti-imane requested a review from msupic February 19, 2025 10:15
@dnestoro
Copy link
Member

dnestoro commented Mar 3, 2025

@msupic please review this pull request when possible as we are planning to have a new release of reachability metadata (+Native Build Tools) soon.

@msupic
Copy link
Contributor

msupic commented Mar 4, 2025

After I reformatted the content of new json metadata files from 6.0.0 and compared with json metadata files from 5.18.1, I found that only difference between those files is in one entry from the reflect-config.json file:

  {
    "condition": {
      "typeReachable": "org.apache.activemq.util.LongSequenceGenerator"
    },
    "name": "org.apache.activemq.util.LongSequenceGenerator",
    "fields": [
      {
        "name": "lastSequenceId"
      }
    ]
  }

@erguiti-imane - could you please confirm that? If that is the case, I think that instead of recreating all metadata for 6.0.0, we can have a simple PR which adds above mentioned entry to the reflect-config.json file 5.18.1

@erguiti-imane
Copy link
Member Author

I had to make some changes to the tests by removing the reliance on the legacy javax.jms API and replacing it with jakarta.jms, which is why I generated new metadata. As we previously agreed, whenever a library starts breaking, we create a new directory and use it for all subsequent versions until another breaking change occurs. This approach helps keep things organized. What do you think, @dnestoro ?

@erguiti-imane erguiti-imane force-pushed the activemq-client/fix-6.0.0 branch from 5a787d3 to 8fd4270 Compare March 27, 2025 15:30
@dnestoro dnestoro removed request for fniephaus and matneu April 7, 2025 09:45
@dnestoro
Copy link
Member

dnestoro commented Apr 7, 2025

I had to make some changes to the tests by removing the reliance on the legacy javax.jms API and replacing it with jakarta.jms, which is why I generated new metadata. As we previously agreed, whenever a library starts breaking, we create a new directory and use it for all subsequent versions until another breaking change occurs. This approach helps keep things organized. What do you think, @dnestoro ?

@erguiti-imane if you changed tests compared to the previous supported version, add another entry (directory for tests and metadata) as we agreed. Only when we have same tests (and you just regenerates metadata), you may just update metadata

@erguiti-imane
Copy link
Member Author

I made the changes to the test class specifically for version 6.0.0, where jakarta.jms is expected. However, for older versions like 5.18.1, ActiveMQ is still based on javax.jms (Java EE 8 / legacy APIs). I tested to confirm, and the changes broke tests for older version.
To maintain compatibility, we need to keep using javax.* for older versions and only use jakarta.* starting from 6.0.0. Hence why separation would be a necessity.

@dnestoro
Copy link
Member

@msupic can you please review this pull request and add comments if something needs to be changed? If there is no issues remain, we should proceed with merging this PR.

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.

3 participants