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

OAK-11247: Migrated groovy test files to Java #1851

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

Conversation

Amoratinos
Copy link
Contributor

@Amoratinos Amoratinos commented Nov 6, 2024

Migrated Groovy test files in oak-pojosr package to Java.

Asserts were changed to use junit assert methods imported statically.

@Amoratinos Amoratinos force-pushed the OAK-11247-migrate-groovy-in-oak-pojosr branch from a03197d to ea0ede3 Compare November 6, 2024 18:13
Removed gmavenplus plugin from pom
Copy link
Contributor

@anchela anchela left a comment

Choose a reason for hiding this comment

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

hi @Amoratinos , i just looked at the SecurityProviderRegistrationTest and that one looks good if we conclude that we want to keep the existing module and the way it's running tests... which is probably what we can achieve with limited effort.
an alternative approach would be to rewrite the intention of the tests within the oak modules they belong to, get rid of the mocking and use PaxExam to build ITs that accurately reflect running oak in a OSGi setup like we have it in Adobe AEM. but maybe that's too much of an effort compared to the gain.

@Amoratinos
Copy link
Contributor Author

use PaxExam to build ITs that accurately reflect running oak in a OSGi setup like we have it in Adobe AEM. but maybe that's too much of an effort compared to the gain.

I'm fine to take that task with me but I think that's a broader scope of the task. Specially from reviewer's point of view it would need to invest more time and effort to go over the changes. If it's fine for you I'll create a new ticket asking to change that test to use PaxExam.

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

I really can't review.

It the Groovy dependency is gone, and integration tests are passing ("-PintegrationTesting"), this should be good.

@reschke
Copy link
Contributor

reschke commented Nov 27, 2024

@AngelaFabregues - paxExam is abandoned; we should not use it in "new" code (see https://mvnrepository.com/artifact/org.ops4j.pax.exam/pax-exam)

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