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 Felix Resolver Tests #548

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Mar 3, 2024

Currently Equinox Embeeds the felix resolver source but does not execute the felix-resolver test.

This is an attempt to also execute the tests to make sure we do not introduce regressions covered by these.

@laeubi laeubi force-pushed the add_felix_test branch 3 times, most recently from c33e385 to 230bc4a Compare March 3, 2024 07:02
@laeubi laeubi requested a review from tjwatson March 3, 2024 07:02
@laeubi laeubi marked this pull request as ready for review March 3, 2024 07:02
@laeubi
Copy link
Member Author

laeubi commented Mar 3, 2024

@tjwatson what do you think? If you think it is useful to have some more tests I'll open a CQ for the imported felix code.

Copy link

github-actions bot commented Mar 3, 2024

Test Results

   87 files  + 9     87 suites  +9   38m 1s ⏱️ +59s
2 198 tests +28  2 151 ✅ +26   47 💤 +2  0 ❌ ±0 
6 726 runs  +84  6 583 ✅ +78  143 💤 +6  0 ❌ ±0 

Results for commit f8285d0. ± Comparison against base commit 49be75d.

♻️ This comment has been updated with latest results.

@tjwatson
Copy link
Contributor

tjwatson commented Mar 4, 2024

@tjwatson what do you think? If you think it is useful to have some more tests I'll open a CQ for the imported felix code.

Would it be better to do the dev work on the resolver in the felix project. I find it easier to do the work there where I can maintain the proper source formatting. Then once I get a PR there and merged I simply copy the relevant source to Equinox.

@laeubi
Copy link
Member Author

laeubi commented Mar 4, 2024

Main problem for me is that Felix has only a few test, equinox has some more and I don't want to break either felix nor equinox. So it seems better to develop the code at Equinox (that seems to have a larger test coverage) and then backport things to Felix, not sure what was the process in the past.

Another obstacle is that felix-devs are not necessarily equinox devs and vice versa ... e.g I'm not a committer there, and there is even not a PR validation at all...

@laeubi
Copy link
Member Author

laeubi commented Mar 5, 2024

@tjwatson as a first step I tried to "modernize" the felix resolver here and add minimal validation to it:

@laeubi
Copy link
Member Author

laeubi commented Mar 5, 2024

This now all builds nice and adds 28 new test, so from my side it would be ready to be merged.

@laeubi laeubi marked this pull request as draft March 5, 2024 12:51
@laeubi
Copy link
Member Author

laeubi commented Mar 5, 2024

Converted to draft as a CQ is possibly needed.

@laeubi
Copy link
Member Author

laeubi commented Mar 6, 2024

@tjwatson If you are fine with it I would create a CQ for this one?

@laeubi
Copy link
Member Author

laeubi commented Apr 21, 2024

@tjwatson any concerns here or should I proceed?

@tjwatson
Copy link
Contributor

@tjwatson any concerns here or should I proceed?

You can proceed. I'm not sure you need a CQ for this since we already had a CQ to pull in the org.apache.felix package from the resolver sub project of felix.

@laeubi laeubi marked this pull request as ready for review April 22, 2024 13:52
@laeubi laeubi merged commit d8cc673 into eclipse-equinox:master Apr 22, 2024
26 of 27 checks passed
@laeubi
Copy link
Member Author

laeubi commented Apr 22, 2024

Then lets merge this as we also don't ship this code is seems safe to me.

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