Fix performance regression in AssertJ recipes #505
Conversation
Since commit openrewrite@368384a , AssertJ recipes are really slower since calling contextSensitive() method disable a cache on JavaTemplate. Fixes: - Make sure there is a static import org.assertj.core.api.Assertions.* , even if not referenced (see openrewrite#491 and openrewrite#479) - reintroduced assertionsParser cache in JUnitAssertEqualsToAssertThat
Since commit openrewrite@368384a , AssertJ recipes are really slower since calling contextSensitive() method disable a cache on JavaTemplate. Fixes: - Make sure there is a static import org.assertj.core.api.Assertions.* , even if not referenced (see openrewrite#491 and openrewrite#479) - reintroduced assertionsParser cache in JUnitAssertEqualsToAssertThat
Since commit openrewrite@368384a , AssertJ recipes are really slower since calling contextSensitive() method disable a cache on JavaTemplate. Fixes: - Make sure there is a static import org.assertj.core.api.Assertions.* , even if not referenced (see openrewrite#491 and openrewrite#479) - reintroduced assertionsParser cache in JUnitAssertEqualsToAssertThat
| rewriteRun( | ||
| //language=java | ||
| spec -> spec.typeValidationOptions(TypeValidation.none()), |
There was a problem hiding this comment.
As it seems like this test passed the type validation before, I wonder why it doesn't anymore.
There was a problem hiding this comment.
That test links to this issue, which was the reason to add contextSensitive as a quick solution at the time, with unfortunate runtime performance impact.
Those custom classes are not available when we drop .contextSensitive(), and the missing types would then break chained recipe runs to for instance adopt more optimal assertion patterns with AssertJ, as part of the migration.
Not quite sure where that leaves this effort. For a one time migration I'd lean towards taking the performance penalty for a more complete / idiomatic migration, but I don't know what prompted the need to fix the performance issue here. Any context there @philippe-granet ?
There was a problem hiding this comment.
I see. The template parameter's type is defined on the source path. That is a rather special case of course. So the test would also fail for a context-sensitive recipe, if the A class were defined in another source file.
I am not sure, but I would probably merge the PR and live with this limitation for now.
Thoughts?
There was a problem hiding this comment.
Ok; I'll trust your judgement on that then and get this merged as is. Thanks a lot both!
There was a problem hiding this comment.
Thanks a lot for noticing, diagnosing and fixing the performance issue with these AssertJ recipes @philippe-granet !
What's changed?
Since commit 368384a , AssertJ recipes are really slower since calling
contextSensitive()method disable a cache on JavaTemplate.Fixes: