-
Notifications
You must be signed in to change notification settings - Fork 36
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
#298: refactoring setting environment variables #1098
#298: refactoring setting environment variables #1098
Conversation
Pull Request Test Coverage Report for Build 13701033161Details
💛 - Coveralls |
cli/src/main/java/com/devonfw/tools/ide/tool/graalvm/GraalVm.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paz-capgemini thanks for your PR. All looks good to me. Great job 👍
I assume that this is in draft mode since you are still working on a JUnit test for this.
I will therefore wait for updates and until you take this out of draft mode.
However, I already give my approval since the main code changes are complete and correct.
I was thinking to only set these variables if they are not already set.
However, I now came to the idea that it would be smarter to create a separate feature/story/PR for that and that we can do that already in [Local]ToolCommandlet
where we call this setEnvironment
method. After PR #1085 is merged, we could simply create a wrapper for ProcessContext as implementation of EnvironmentContext that implements withEnvVar
such that only if the variable is not already defined, it will be set in the ProcessContext.
So we leave this out of this PR.
The linux test for Aws, or to be more precise testing the symlink part seems to be a bit more complicated. That applies to the Azure test as well since inside the Even on linux itself, the test fails due to the following problem: If the folder already exists:
If the folder does not exist:
After some consideration with @jan-vcapgemini , we came to the conclusion that the complexity is quite high to resolve this and the benefit is not that large. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paz-capgemini thanks for also adding the test for AWS slightly increasing our coverage. Great job!👍
For the record: AwsTest.testAwsSetEnvironment
is also performing the installation of AWS. It could therefore be merged with testAwsInstall
to avoid "waste". I know that it is also beneficial to have dedicated tests for smaller aspects, but all these tests using newContext
are recursively copying a directory structure and initialising the context with it what is slow compared to a plain class test. Since this is just a personal flavour and there is absolutely nothing wrong with your test, I merge it as is. Currently our CI times are still good enough and we do not yet need to squeeze every second we can out of the build times.
Fixes #298