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

Fix running timezone-sensitive tests in IntelliJ IDEA #358

Merged
merged 1 commit into from
May 30, 2023

Conversation

ksobolew
Copy link
Contributor

@ksobolew ksobolew commented May 25, 2023

Some time ago, some properties were moved to argLine because Surefire incorrectly passes the user.language and user.region system properties to the tests. At that time, the user.timezone also got along for the ride, but it turns out, that that latter property is passed correctly. So while it works for Maven and Surefire, moving it into argLine causes IntelliJ IDEA to not pick it up, which in turn means that some tests are not working properly when executed inside the IDE. We can move it safely back, as it improves quality of life.

This reverts parts of commit d4630aa.

airbase/pom.xml Outdated Show resolved Hide resolved
@ksobolew ksobolew force-pushed the kudi/fix-system-props-again branch from 00819ad to 973990a Compare May 29, 2023 12:22
@findepi
Copy link
Contributor

findepi commented May 29, 2023

thanks

update CHANGES too

@ksobolew ksobolew force-pushed the kudi/fix-system-props-again branch from 973990a to cb4feb2 Compare May 29, 2023 12:38
@ksobolew
Copy link
Contributor Author

update CHANGES too

Done

@ksobolew ksobolew force-pushed the kudi/fix-system-props-again branch from cb4feb2 to e3a8945 Compare May 29, 2023 12:40
@ksobolew
Copy link
Contributor Author

update CHANGES too

Done

...under the correct version now

CHANGES.md Outdated
@@ -1,3 +1,7 @@
Airbase 140

* Pass `user.timezone` in tests as a system property, not as a JMV argument
Copy link
Contributor

Choose a reason for hiding this comment

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

what does JMV stand for?

also mention that this supposedly restores IntelliJ ability to set timezone correctly when running tests from within that IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what does JMV stand for?

Oops. JVM, obviously.

also mention that this supposedly restores IntelliJ ability to set timezone correctly when running tests from within that IDE.

Done.

@ksobolew ksobolew force-pushed the kudi/fix-system-props-again branch from e3a8945 to 6f596e0 Compare May 29, 2023 14:02
@martint
Copy link
Member

martint commented May 29, 2023

Can you update the commit message to say “fix xxxx”? You can mention that it’s a partial revert of something else in the commit message, but that’s unnecessary for the title of the commit.

@ksobolew ksobolew force-pushed the kudi/fix-system-props-again branch from 6f596e0 to 6a9a23c Compare May 29, 2023 14:43
@ksobolew
Copy link
Contributor Author

Can you update the commit message to say “fix xxxx”? You can mention that it’s a partial revert of something else in the commit message, but that’s unnecessary for the title of the commit.

Sure; done

@findepi
Copy link
Contributor

findepi commented May 29, 2023

the new commit title is

Fix passing timezone information to Surefire

which isn't exactly 100% correct.

we were passing timezone information to surefire correctly
the fix is for Intellij, afaiu

Some time ago, some properties were moved to `argLine` because Surefire
incorrectly passes the `user.language` and `user.region` system
properties to the tests. At that time, the `user.timezone` also got
along for the ride, but it turns out, that that latter property *is*
passed correctly. So while it works for Maven and Surefire, moving it
into `argLine` causes IntelliJ IDEA to not pick it up, which in turn
means that some tests are not working properly when executed inside the
IDE. We can move it safely back, as it improves quality of life.

This reverts parts of commit d4630aa.
@ksobolew ksobolew force-pushed the kudi/fix-system-props-again branch from 6a9a23c to 04a382a Compare May 30, 2023 08:33
@ksobolew ksobolew changed the title Partially revert "Fix the way locale information is passed to Surefire" Fix running timezone-sensitive tests in IntelliJ IDEA May 30, 2023
@ksobolew
Copy link
Contributor Author

we were passing timezone information to surefire correctly
the fix is for Intellij, afaiu

You're right, I changed the title and reworded the commit message a bit too.

@findepi findepi merged commit 02901c1 into airlift:master May 30, 2023
@findepi
Copy link
Contributor

findepi commented May 30, 2023

Merged, thanks!

@ksobolew ksobolew deleted the kudi/fix-system-props-again branch May 30, 2023 12:16
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