Skip to content
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

Add o.e.e4.core.javax.tests as javax-using copy of o.e.e4.core.tests #791

Merged

Conversation

HannesWell
Copy link
Member

and migrate o.e.e4.core.tests to use jakarta-annotations.

All test-cases besides the About-tests are copied from org.eclipse.e4.core.tests
Consequently o.e.e4.core.javax.tests only differ in the fact that the former uses javax-annotations, while the latter uses jakarta-annotations.

When support for javax-annotations is removed from the E4-Injector, o.e.e4.core.javax.tests can be deleted too.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2023

Test Results

     534 files  +    3       534 suites  +3   1h 2m 39s ⏱️ - 13m 6s
  3 774 tests ±    0    3 769 ✔️ ±    0    5 💤 ±0  0 ±0 
11 943 runs  +603  11 907 ✔️ +600  36 💤 +3  0 ±0 

Results for commit ed1e24a. ± Comparison against base commit 00e9088.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member Author

What is the opinion from the others about this?

It adds quite some lines of code (but since it is a copy of existing Eclipse Code I assume a CQ is not necessary).
Nevertheless I think it is mandatory to ensure a contentious functionality of the E4 Injector with both jakarta and javax annotations, with and without #792.

For #792 the new copied tests already detected a flaw with the handling of provides under javax (which I have to investigate).

@HannesWell HannesWell force-pushed the javax-injection-tests branch from 917ffb2 to 4177b01 Compare October 30, 2023 20:24
@mickaelistria
Copy link
Contributor

Yes, I think this critical change and the strong need for backward compatibility really makes the extra code worth it.
And indeed, no CQ is necessary for code that was already approved.

and migrate o.e.e4.core.tests to use jakarta annotations.

All test-cases, besides the About-tests, are copied from
org.eclipse.e4.core.tests, before the migration of the latter.
Consequently o.e.e4.core.javax.tests only differ in the fact that the
former uses javax annotations, while the latter uses
jakarta annotations.

When support for javax annotations is removed from the E4-Injector,
o.e.e4.core.javax.tests can be deleted too.
@HannesWell HannesWell force-pushed the javax-injection-tests branch from 4177b01 to ed1e24a Compare October 30, 2023 21:38
@akurtakov
Copy link
Member

Please land it in - we have to be sure that both javax and jakarta works for at least 2 more years (starting from the date we announce javax being deprecated).

@HannesWell HannesWell merged commit 4fd12ad into eclipse-platform:master Oct 31, 2023
14 checks passed
@HannesWell
Copy link
Member Author

Please land it in

Done. Thank you and Mickael for your partizipation.

@HannesWell HannesWell deleted the javax-injection-tests branch October 31, 2023 09:03
@Bananeweizen
Copy link
Contributor

@HannesWell where should the jakarta imports come from? I have multiple platform IDEs which all fail to compile because that's not available anywhere, even after triggering Oomph.
grafik

@HannesWell
Copy link
Member Author

@HannesWell where should the jakarta imports come from? I have multiple platform IDEs which all fail to compile because that's not available anywhere, even after triggering Oomph.

They are in the platform-TP and should be in the I-builds repo.
So everything should work an these annotationens are already required by platform for a few weeks.

I can check my Installations later this day when I have computer access again.

@merks
Copy link
Contributor

merks commented Nov 1, 2023

Note that there are two versions of this bundle and only the 2.x version exports jakarta namespace.

image

Both are in the resolved target platform:

image

@Bananeweizen
Copy link
Contributor

Thanks @merks and @HannesWell, I got it working after some more rounds of restart and explicitly triggering Oomph. Not sure what I missed the first time. Sorry for the noise.

@merks
Copy link
Contributor

merks commented Nov 1, 2023

No problem! I'm glad you ironed it out...

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.

5 participants