-
Notifications
You must be signed in to change notification settings - Fork 695
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
test: Refactor time tests #1880
Conversation
- Move testCurrentDateTimeFunction from JodaTimeDefaultsTest to JodaTimeTests - Rewrite testDefaultCurrentDateTime in JodaTimeDefaultsTest - Add testDefaultCurrentDateTime to Java and Kotlin tests
get() = | ||
if (TransactionManager.isInitialized() && TransactionManager.currentOrNull() != null) { | ||
currentDialectTest | ||
} else null |
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.
Not about the PR, but I'm confused about our Detekt GH action.
Based on the MultiLineIfElse
setting in detekt-config
, shouldn't this line be disallowed? It's flagged in the IDE.
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.
I would also make an extension for this condition
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.
Not about the PR, but I'm confused about our Detekt GH action.
Based on the
MultiLineIfElse
setting indetekt-config
, shouldn't this line be disallowed? It's flagged in the IDE.
This is in the exposed-tests
module, which is excluded in the detekt GitHub action. I do have it on my to-do list to remove all the detekt warnings in that module at some point and include it in the GitHub action, but I'll create an issue for it.
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.
That's on me. I should've realised that code wasn't a part of any datetime module.
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.
I would also make an extension for this condition
I'll work on this in the next PR for refactoring some code in the exposed-tests
module.
LGTM |
testCurrentDateTimeFunction
fromJodaTimeDefaultsTest
toJodaTimeTests
testDefaultCurrentDateTime
inJodaTimeDefaultsTest
testDefaultCurrentDateTime
to Java and Kotlin tests