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
RFC: kondo lint to enforce naming convention for thread not safe defn/defmacro in tests #37126
Conversation
… end with exclamation
1 failed test on run #1097 ↗︎
Details:
Review all test suite changes for PR #37126 ↗︎ |
@@ -595,11 +595,11 @@ | |||
metabase.related-test/with-world macros.metabase.related-test/with-world | |||
metabase.shared.util.namespaces/import-fn macros.metabase.shared.util.namespaces/import-fn | |||
metabase.test.data.users/with-group-for-user macros.metabase.test.data.users/with-group-for-user | |||
metabase.test.util/with-temp-env-var-value macros.metabase.test.util/with-temp-env-var-value |
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 changed this one macro as an example
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.
Big fan of this.
The main thing that stopped me from going any further with my previous PR was just that so many things needed to be fixed -- maybe we can have the linter ignore some of the test helpers that we know are missing !
for the time being, and we can fix them in follow-on PRs.
We should also make sure this respects #_:clj-kondo/ignore
forms, because there are some legitimate cases where you might want to use with-redefs
in something without marking it as !
, for example functions that respect test-helpers-set-global-values!
. Normally they would be thread-safe, but then they'd have a thread-unsafe behavior if used inside test-helpers-set-global-values!
. That's ok tho because test-helpers-set-global-values!
itself ends in a !
|
@camsaul yes, it does work if with |
@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
This adds a lint to force
defn / defmacro
to have a name that ends with!
if it contains not thread safe calls likemt/with-temp!
.This is the first step to pursue this RFC from Cam. The goal here is to make identifying "parallelable" tests easier. To achieve that, we should adopt a convention name for any function / macro that cannot be used in parallel tests to have the name end with
!
.Once we have this, it'll be easy to write a kondo to suggest whether or not a deftest can be paralleled.
This lint will only be used in test namespaces.
One more thing, the reason I enforce this lint on
defn
is because most of the time, we write macros in this patternso, to make sure we cover all macros, we should make all functions recognizable as safe / not safe.