+1 As someone who reviews Druid release candidates but does not develop Druid code, I wish that “mvn test” would “just work”. I can imagine that first-time and occasional contributors are in a similar boat to me.
I know this is not an easy thing to achieve, because the test suite also has to work for more experienced contributors. Julian > On Apr 25, 2019, at 11:42 AM, David Glasser <glas...@apollographql.com> wrote: > > As a new contributor, I've actually really appreciated the careful and high > quality code reviews I've received (primarily from Jihoon). > > The real source of friction for me has not been the feedback from > developers, but the painfulness of running the tests. > > - Figuring out how to locally run the full suite of tests that CI will run > is not super documented. > - Understanding how to just run the tests that are most relevant to your > change (either from CLI or IntelliJ) isn't well documented. It's > especially unclear what things can be perfectly successfully run from > IntelliJ's own test runners and what things really require you to run them > through Maven. (Druid is also the first time I've used Maven directly and > it's a huge challenge for me to understand what's going on and how to run > it properly.) > - Many of the tests are just very slow and maybe could be parallelized > better? > > It's enough of a pain that I often when iterating wouldn't even bother > trying to run tests manually but would just push to GitHub and let Travis > run it instead. But Travis itself is very slow and while it's somewhat > parallelized, it's not super parallelized — and as a lowly PR author I > can't even kill the overall job if I push a new version of the PR or if a > sub-job already failed. So this would frequently add "round trip" times of > half an hour or more in the middle of trying to take the good advice from > reviewers to heart. > > So while I agree that it would be great to move as many of the checks as > possible into CI from "core dev teams mind", I'd also encourage folks with > more time and expertise than I have now to put effort into making the CI > experience faster and easier. (I wonder if some of the issues could just > be solved with money, eg running on more powerful Travis machines or > parallelizing the slower tests even further onto a larger number of > containers. I've also generally found in my own work that CircleCI at > least feels faster and easier to work with than Travis FWIW.) > > --dave > > On Mon, Apr 22, 2019 at 7:44 PM Gian Merlino <g...@apache.org> wrote: > >> Hey Druids, >> >> Sometimes I feel like this too: >> https://twitter.com/julianhyde/status/1108502471830204416. >> >> I believe our code review process today has too much friction in it, and >> that we can work to reduce it. The two main frictions I see are code >> reviews not happening in a timely manner, and code reviews sometimes >> descending into a swamp of nit-picks. The good news, at least, is that we >> are not the first development team to have these problems, and that they >> can be solved. I've written some thoughts below about principles that I >> think might help. I am not necessarily proposing making these the law of >> the land, but am hoping to start a discussion about how we can generally >> improve things. >> >> 1) "Let robots handle style checks." Encode Druid's code style in >> checkstyle or other tools, and avoid making subjective style comments >> directly as humans. If we can't figure out how to set up automated >> verification for a particular style point, then give it up. Rationale: >> authors can self-check style when checking is automated. Also, it's better >> for robots to enforce style, because software development is a social >> endeavor, and people don't mind style nit-picking from robots as much as >> they do from humans. >> >> 2) "Write down what really matters." I suggest we put together a short, >> highly visible list of things that should be considered commit-blockers for >> patches. Not a list of best practices, but a list of true blockers. "Short" >> is in service of point (3), below. "Highly visible" is so it can act as a >> shared set of values. My suggested list would be correctness of >> implementation, documentation of new or changed functionality, tests for >> reasonably testable functionality, avoidance of excessive additional >> maintenance burden, and avoidance of breaking existing use cases (including >> things that would break clusters that run at large scale, or that would >> break rolling updates). Some of these points are subjective but I think >> it's still a good start. Rationale: authors will know what is expected of >> them, which should generally improve PR quality, and speed up review. Also, >> similar to the previous point: people are generally happy to follow what >> they perceive as community-maintained standards, but not as happy to follow >> what they perceive as the opinion of a single reviewer. >> >> 3) "Minimize hoop-jumping." We should make an effort to avoid the '74 >> one-line emails' problem. Endeavor to make fewer and fewer comments as the >> number of rounds of review of a PR gets higher. Endeavor to keep the number >> of rounds of review from getting too high. Authors can help here by >> explaining their decisions clearly, and by avoiding making large changes in >> their patches after review has started. Reviewers can help by making their >> first review as comprehensive as possible, to avoid the need to bring up >> brand-new points on later rounds of review. Reviewers can also help by >> writing out changes they're asking for explicitly (suggest a new comment, >> doc wording, method name, etc). Reviewers can also help by noting in a >> review comment when a suggestion is just a suggestion -- useful because >> many authors are likely to interpret an unqualified comment as a commit >> blocker. Rationale: the more rounds of review a PR goes through, and the >> more a review feels like a collection of disconnected 1-liner comments, the >> more it makes the original author feel as if he or she is being made to >> jump through hoops. This makes people less likely to contribute in the >> future, and damages efforts to build community. >> >> 4) "Pitch in on reviews." A relatively small number of committers are doing >> most of the code reviews. These people have limited time, and it means that >> PRs often stay in the review queue for a while without anyone looking at >> them. Any committer can pitch in, and in fact, even non-committers can >> (although their votes are nonbinding, their reviews are still helpful for >> social reasons). So anyone interested in reviewing is encouraged to do so. >> Rationale: improves bandwidth, prevents burning out the small number of >> volunteers that participate in reviews. >> >> Looking forward to seeing what people in the community think. >> >> Gian >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
