jacopoc commented on PR #916: URL: https://github.com/apache/ofbiz-framework/pull/916#issuecomment-3615701468
Hi Gaetan, I didn’t review your pull request in detail because I don’t have deep knowledge of the original Java tests that you reimplemented in Groovy. However, I agree with the general direction and approach when dealing with legacy code: if there are opportunities to simplify old designs, it’s reasonable to introduce some simplifications, even at the cost of removing minor features, in order to achieve a more modern and maintainable codebase. In particular, I agree with your decision to remove the GenericTestCaseBase class and the useAllMemory method. I do have one concern about useJUnit(), which, if I’m not mistaken, configures Gradle to use JUnit 4. I believe there are some Groovy tests that rely on JUnit 5, so this change might prevent them from running. More generally, it would be helpful to better understand the underlying reasons for the issues we encountered, as this knowledge could help us improve our testing framework setup. Anyway, thanks for your work! In my opinion, we can accept this pull request if you feel confident about it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
