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 JDK Modules Export to JUnit Test to run Integration Tests via IDE #864

Merged
merged 7 commits into from
Dec 23, 2023

Conversation

StefanKruk
Copy link
Collaborator

@StefanKruk StefanKruk commented Dec 21, 2023

To Run IntegrationTests via IntelliJ IDEA it is necessary to open some java Modules.
Via Ant this is done by reading the Property standalone.jdkmodulesexports from advanced.properties and add them as compilerarg (util.xml) or jvmargs (testing.xml)

When running Integration Test without those statements, the Test will fail with several BeanCreationException

Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make protected native java.lang.Object java.lang.Object.clone() throws java.lang.CloneNotSupportedException accessible: module java.base does not "opens java.lang" to unnamed module @8e0379d

It is possible to add those Configuration to the JUnit Run Template so that every other Test is run directly with it, but if new Developers are starting to use the Plugin or the Project needs to be setup again and this setting is forgotten, Integration Tests will fail to run.

Therefore I updated HybrisJUnitExtension to read the property from advanced.properties and add them as vmParameters (if not already existing) so that it will be easier to run Integration Tests from now on.

Signed-off-by: Your Real Name [email protected]

@mlytvyn mlytvyn added this to the Release 2023.3.3 milestone Dec 21, 2023
addJdkPropertiesIfNecessary(vmParameters, project);
}

private void addJdkPropertiesIfNecessary(final ParametersList params, final Project project) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you try to rely on PropertiesService to get required property?

Copy link
Collaborator Author

@StefanKruk StefanKruk Dec 21, 2023

Choose a reason for hiding this comment

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

I will change the Implementation to use the PropertiesService.

Short question regarding this. In the Service is the method findAllProperties which currently not supportes all possibilities to set properties.

First of all the Optionl Config dir can also be set via HYBRIS_OPT_CONFIG_DIR Environment Variable (see https://help.sap.com/docs/SAP_COMMERCE/b490bb4e85bc42a7aa09d513d0bcb18e/8beb75da86691014a0229cf991cb67e4.html)

There is also the possibility to define Properties by specifying a File via Environment Property HYBRIS_RUNTIME_PROPERTIES . This Property file is loaded before the HYBRIS_OPT_CONFIG_DIR

And finally there is the possibility to define Environment Properties for setting Values (see: https://help.sap.com/docs/SAP_COMMERCE/b490bb4e85bc42a7aa09d513d0bcb18e/8b8e13c9866910149d40b151a9196543.html?q=y_#using-environment-variables-instead-of-files-for-secure-settings)

If desired I can add the Support for those as well so that we will be able to get most of the Properties a User can define.

UPDATE
It also seems project.properties Files are not considered right now as well or am I wrong?
Further is the method resolvePropertyValue anywhere used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@StefanKruk , this service was introduced by original authors years ago. This service was initially used for ImpEx files as a resolver for $config-... properties.

All your comments are valid and it would be great to see them implemented!

p.s. you've missed one very important change, please add yourself to the CONTRIBUTING.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will first change my current implementation for the Integration Tests as you have suggested and will finalize this PR.
Afterwards I will try to update the Properties Loading. I will most likely create a new PR for this.

@StefanKruk
Copy link
Collaborator Author

Thanks for the Input. I will try to get the changes done.

@mlytvyn mlytvyn changed the base branch from main to release/2023.3.3 December 23, 2023 12:11
@mlytvyn
Copy link
Collaborator

mlytvyn commented Dec 23, 2023

@StefanKruk , great job, well done!

If you would like to participate in any other development feel free to contact me in the Slack, there are few ideas, like:

@mlytvyn mlytvyn merged commit 01bcd6b into epam:release/2023.3.3 Dec 23, 2023
1 check passed
@StefanKruk StefanKruk deleted the fix/JunitIntegrationTests branch December 23, 2023 14:22
@StefanKruk
Copy link
Collaborator Author

Will Join Slack as soon as JetBrains will send me an invite. They are stating die to technical issues they need to send the invite by hand

@mlytvyn
Copy link
Collaborator

mlytvyn commented Dec 24, 2023

@StefanKruk , actually, I'm referring to the Plugin's Slack channel :)

https://github.com/epam/sap-commerce-intellij-idea-plugin

image

@StefanKruk
Copy link
Collaborator Author

StefanKruk commented Dec 24, 2023

Actually the slack link is telling me "workspace not found"
If I open the link in browser first (on mobile) and then click join, the workspace was found, but as I do not have an epam email I need an invite.

Please invite me: [email protected]

@mlytvyn
Copy link
Collaborator

mlytvyn commented Dec 24, 2023

changed authentication, can you try now?

@StefanKruk
Copy link
Collaborator Author

Still no luck, now it is telling me I need a password but I do not have one and also no account associated with the workspace, also the invite link behind the badge is now invalid.

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