I think Dan's framework of thinking is right -- what is the probability of something finding a real issue, vs. the cost of running that all the time.
Obviously, we cannot run *everything* all the time. There's an infinite number of things to run and infinite matrix of configurations. Many tests are not even possible to execute locally like testing scale, performance, compatibility across platforms, etc. So, we run locally *the least*, slightly more in presubmit, slightly more in post-submit, even more nightly, and as much as possible for each release. Of course, it is great to catch issues as soon as possible, but the cost is prohibitive in terms of time and productivity. This may sound a bit tangential so far... but, I think Aviem's (and others) point of view come from a social aspect. As a contributor, nobody wants to be "embarrassed" by proposing a pull request that is broken and has obvious problems. One might think of themselves as failing to do a good job in this particular case. So, instead, I might want to default to running more things locally to prevent the "embarrassment". Looking more broadly, however, there's a huge difference in things that the default local execution *and* pre-commit don't exercise. It is really huge and many breakages are caught in post commit, nightlies, or, at the very end, cherrypicked into the release. So, I'd like to argue that nobody should ever be embarrassed by proposing a pull request. It is normal, expected, reasonable that Jenkins will catch issues -- that's why we have it. Instead, think in terms of "error budget" -- increase individual productivity by sacrificing occasional breakages. It is also completely normal that a PR causes a post-commit breakage at times, or that a release is delayed because of some PR -- this is all normal and fits into your "error budget". One easy improvement that I know Jason is looking at is to separate the precommit signal into multiple signals. So, instead of a one "red" signal, contributors can get five "greens" and one "red", which may help decrease this social impact. Hopefully, this is a convincing argument to use this framework in deciding this matter. On the lower-lever issue, I think rat and findbugs have a low probability of finding an issue in most cases. (Also, an explicit +1 to Kenn's point of view of getting people to the PR so we can work with them, as opposed to blocking them locally before they interact with us.) On Fri, Feb 10, 2017 at 11:50 AM, Kenneth Knowles <k...@google.com.invalid> wrote: > Since the discussion has returned to the thread rather than Dan's PR, I > want to paraphrase the point I feel strongest about here: > > *For a new contributor, I want to minimize the distance between them > deciding to hack and becoming our friends.* > > So I don't want them to have to learn much, if anything, about our idioms > prior to opening a PR. That includes checkstyle, findbugs, javadoc, RAT, > (any others?). Once a PR is open, we can welcome them to the community, > help them understand any failure that happened via Jenkins, tell them the > command to run the more thorough test, or even issue little fix PRs against > their branch. If they get blocked by nitpicky or confusing failures in > their console or while hacking, they might (reasonably, IMO) decide to go > do something else. > > Folks other than newcomers can learn a repertoire of commands, like Robert > says. So we shouldn't consider them (aka "us") so much when deciding > whether "fast" or "slow" is the default, as long as we can explicitly > choose. > > Kenn > > On Fri, Feb 10, 2017 at 11:00 AM, Robert Bradshaw < > rober...@google.com.invalid> wrote: > > > > 1. Developer productivity -- Jenkins should run many more checks than > > > developers do. Especially time-, resource-, or setup- intensive tasks. > > > 2. Automated enforcement -- Jenkins is better at running the right > > commands > > > than we are. > > > 3. Lower the barrier to entry -- individual developers need not have a > > > running Spark/Flink/Apex/Dataflow setup in order to contribute code. > > > 4. Focus on the user -- someone checking out the code and using it for > > the > > > first time does not care whether the code style checks or has the right > > > licenses -- that should have been enforced by the Beam team before > > > committing. > > > > > > We should be *very* choosy about what we enforce on every developer > every > > > time they go to compile. I probably compile Beam 50x-100x a day. > > Literally, > > > the extra minutes you want to add here will cost me an hour daily. > > > > > > > By the same token of having a different bar for the Jenkins presubmit vs. > > what's run locally, I think it makes a lot of sense to run a different > > command for iterative development than you run before creating a pull > > request. E.g. during development I'll often run only one test rather than > > the entire suite, but do run the entire suite occasionally (often before > > commit, especially before pushing). > > > > The contributors guild should give a suggested command to run before > > creating a PR, right in the docs of how to create a PR, which may be more > > expensive than what you run during development. IMHO, this should be > fairly > > comprehensive (certainly tests and checkstyle, maybe javadoc and > findbugs). > > This should be the "default" command that the one-time-contributor should > > know. For those compiling 50x or more a day, I think the burden of > learning > > a second (or more) cheaper commands is not high, and we could even put > such > > a thing in the docs (and hopefully a common maven convention like "mvn > > test"). > > > > Kenn > > > > I've listed the fraction of commits I think will break one of the > following > > > if that property is not tested: > > > > > > * compiling (100%) > > > * tests (100%) > > > * checkstyle (90%) > > > * javadoc (30%) > > > * findbugs (5%) > > > * rat (1%) > > > > > > So you can see where I stand and why. I'm sorry that 1/20 PRs has > Apache > > > RAT catch a licensing issue or Findbugs catch a threading issue -- you > > can > > > always get a larger set of the precommit checks using -Prelease, though > > of > > > course the integration tests and runnableonservice tests may catch more > > > issues still. But I want my developer minutes back for the 95%+ of the > > > rest. > > > > > > Dan > > > > > >
