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

property override fix #2474 #2510

Merged
merged 1 commit into from
Feb 18, 2024
Merged

Conversation

dvargas46
Copy link
Contributor

Description

Minimal solution to fix the issue of overriding runner properties by JVM args.

@dvargas46
Copy link
Contributor Author

@ptrthomas this PR removes setting the System property from the systemProperty method on the runner to fix this issue. Otherwise the other fix I considered of "syncing" the karate properties with the System properties ended up breaking other tests. Since it lead to a case where, in a multi-runner execution, one runner setting a property would override all other runners that needed to set the same property - e.g., if server.port is set on runner 1, then subsequent runners could not set the same property anymore and it would be forced to use whatever the first runner set it to unless it was explicitly cleared first.. I don't think that would be desirable, especially for mocks where the runners can use this method to set server port details.

If setting the System property is still needed in the systemProperty method for something else, then this issue may become more challenging to deal with. My first thought would be to cache the System properties initially, so that we could have a reference of what properties came from JVM arguments and use that to override each time the runner needs to reference it, while still maintaining an actual sync across runner and system properties elsewhere. Let me know if you have any thoughts on which direction we should take here.

@ptrthomas ptrthomas merged commit 970c7d5 into karatelabs:develop Feb 18, 2024
1 check passed
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