asolimando commented on PR #4648: URL: https://github.com/apache/calcite/pull/4648#issuecomment-3566327905
> > I want to be more explicit here: it's ideal to track what change led to this improvement but not a necessary condition, it's great to know the problem doesn't hold anymore and having tests to catch regressions, even if we couldn't isolate the source of the fix. > > This said, given the nature of the PR, we aren't in a rush for merging it and we can wait for Mihai's feedback. > > Thanks for your diligence and precision @xiedeyantu, much appreciated! > > Thank you for your feedback. I actually have a question and was wondering if you could give me some advice. If there's an old Jira ticket documenting an issue that has already been fixed in the current version, is it necessary to submit a PR containing only the tests, as in the current PR? My feeling is that a PR adding the repro as test and making sure it passes and that the expected result is "valid" (which depends on the nature of the issue at hand) is the right thing to do and sufficient to close a ticket. Cherry on top is to identify what fixed and how, but not strictly needed. Such a PR closes an open issue and it confirms it's been resolved, so no contributor spend more time on it, and the added tests enrich our test suite, improve coverage and protect from regressions, net improvements. What reproducers, where etc follows the same rule as for adding any other kind of tests, not specific to this situation IMO. In the past I have even added some tests I created to verify some non-trivial behavior I wasn't sure about, which wasn't covered by existing tests. Tests act as documentation too in a sense. -- 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]
