paul-rogers commented on code in PR #12346: URL: https://github.com/apache/druid/pull/12346#discussion_r887427424
########## .github/pull_request_template.md: ########## @@ -11,7 +11,7 @@ https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and ### Description -<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. --> +Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. Focus on "User Experience", describe how users will interact with the changes you're submitting. It's important to get the user experience right, because if we release a feature with poor UX, then it's hard to change later without breaking compatibility. We'd like the master branch to be releasable at all times, which means we should figure UX out before committing a patch. Review Comment: As a contributor, I mostly eliminate sections. In this case, if we make lower-level changes (such as to the execution engine), the user experience affect is minimal. We'd end up writing "the user will no longer experience such-and-so bug." Or, "when running such-and-so-query, performance will be a bit faster." That kind of info is not super-helpful. Suggestion, say something like: > If this is a UI issue, or primarily a user-facing issue, please update the relevant documentation. That documentation change will explain to the reviewer the affect of your change. Otherwise, the change is pretty well described by all the cruft already in the template: what did you change? Why? What else could you have done? How did you test it? Etc. ########## .github/pull_request_template.md: ########## @@ -33,7 +33,12 @@ In each section, please describe design decisions made, including: <!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. --> -<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. --> +<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. +--> + +It's important to get tests right too because they ensure high quality releases, and they also ensure that future changes can be made with low +risk. Make reviewer's life easier by adding good testing of the new stuff and writing out (in natural language) why you think your tests cover all the important cases. These natural language descriptions are *very helpful* for reviewers, especially when they add context to the patch. Don't make reviewers reverse-engineer your code to guess what you were thinking. Review Comment: This is all true, but the PR template is not the place to put this into: us contributors will have to delete it over and over. Can we put this into a description for how to make contributions? If the contributor provided unit tests, they will speak for themselves. As a reviewer: it is the first thing I check. If tests are missing, that's a "hey, can you add some tests" blocker right there. In the docs, we should explain the kinds of tests, which is something you'll want help on. There are the simple tests. Then there are the mock-heavy test because things won't run in a test: perhaps we can help folks understand what should be mocked and when, and where to look for existing examples. Then there are integration tests (ITs) which are currently very light, and which are really the only way to test the entire Druid server. They are presently a bear to use (we're trying to fix that in another PR), but for now, people just gotta set aside a couple of days and power through the mess. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
