Re: Add code quality checks to pre-commits.

Hi everyone, I've summarized discussion so far into proposal doc , so that it gets easier to comment upon. Please add your comments. Link explicitly: https://docs.google.com/document/d/1YbV18mrHujmiLBtadS1WzCVeiI3Lo

Re: Add code quality checks to pre-commits.

Currently I d opt for just enabling jacocoTestReport on javaPreCommit. Although this will disable the build cache, it seems we could consider that to be effectively disabled anyway. E.g. looking into https://scans.gradle.com/s/eglskrakojhrm/performance/buildCache we see a local cache miss rate of

Re: Add code quality checks to pre-commits.

I don't think it would be unreasonable to disable parallel build for code coverage if necessary, perhaps in a separate Jenkins job if it significantly increases job time. +1 A separate coverage job would be helpful. It would be fine if it ran longer than the regular PreCommit. On 08.01.19 1

Re: Add code quality checks to pre-commits.

I started a PR to see what it would take to upgrade to Gradle 5.0: https://github.com/apache/beam/pull/7402 It seems the main blocker it gogradle plugin compatibility for the Go SDK. The gogradle project is actively working on compatibility, so perhaps we could check back in a month or so: https:/

Re: Add code quality checks to pre-commits.

I am pretty sure the build cache is not in use on Jenkins. It would have to live somewhere persisted across runs, which I don't think is default. And we'd be seeing much lower build times. Kenn On Mon, Jan 7, 2019 at 9:42 AM Mikhail Gryzykhin wrote: > @Jason > I believe that we want to get some

Re: Add code quality checks to pre-commits.

@Jason I believe that we want to get some coverage report as a PR pre-commit check, so that we can utilize it as part of code review. Removing parallelization increases pre-commit duration greatly (see the big drop on this graph

Re: Add code quality checks to pre-commits.

Michael, could we solve that problem by adding an additional build with those features disabled specifically for generating coverage data over time? Any tradeoffs to doing something like that? On Mon, Jan 7, 2019 at 8:23 AM Michael Luckey wrote: > Unfortunately I am a bit worried, that enabling

Re: Add code quality checks to pre-commits.

Unfortunately I am a bit worried, that enabling code coverage will not come for free at the moment. As far as I understand the current state of the build system, we would either have to disable parallel builds or the build cache, which I assume both to be enabled currently on precommit runs. I am t

Re: Add code quality checks to pre-commits.

Code coverage would be very useful, regardless of what tool we use to measure it. Especially when reviewing PRs, because it provides a quantitative measure which can help reviewer and contributor to decide if testing is sufficient. I'd prefer to use specific tools instead of one-fits-it-all too

Re: Add code quality checks to pre-commits.

I think we should also consider false positive ratio of the tool. Oftentimes deeper analysis easily produces tons of false positives which make people less interested in static analysis results because of triaging overheads. On Thu, Jan 3, 2019 at 4:18 PM Scott Wegner wrote: > Discussion on soft

Re: Add code quality checks to pre-commits.

Discussion on software engineering practices and tools tends to gather many opinions :) I suggest breaking this out into a doc to keep the discussion organized. I appreciate that you've started with a list of requirements. I would add a few: 6. Analysis results should be integrated into the code

Re: Add code quality checks to pre-commits.

Let me summarize and answer main question that I see: 1. Seems that we do want to have some statistics on coverage and integrate automatic requirements into our build system. 2. Implementation is still to be discussed. Lets talk about implementation further. My requirements for choice are: 1. Too

Re: Add code quality checks to pre-commits.

It would be very useful to have line and/or branch coverage visible. These are both very weak proxies for quality or reliability, so IMO strict thresholds are not helpful. One thing that is super useful is to integrate line coverage into code review, like this: https://docs.codecov.io/docs/browser-

Re: Add code quality checks to pre-commits.

I don't have any experience of using SonarQube but Coverity worked well for me. Looks like it already has beam repo: https://scan.coverity.com/projects/11881 On Thu, Jan 3, 2019 at 1:27 PM Reuven Lax wrote: > checkstyle and findbugs are already run as precommit checks, are they not? > > On Thu,

Re: Add code quality checks to pre-commits.

checkstyle and findbugs are already run as precommit checks, are they not? On Thu, Jan 3, 2019 at 7:19 PM Mikhail Gryzykhin wrote: > Hi everyone, > > In our current builds we (can) run multiple code quality checks tools like > checkstyle, findbugs, code test coverage via cubertura. However we do

Re: Add code quality checks to pre-commits.

+1 to enabling warnings first. This will allow us to evaluate how much value we get and the frequency of false-positives / noise. I'd be curious to hear from those that've worked in other projects on experiences with SonarQube or other tools. At Google, code coverage is integrated into the code r

Re: Add code quality checks to pre-commits.

I had the same question, and tt supports many more than we do: https://www.sonarqube.org/features/multi-languages/ All the various rules checks have clear explanations and justifications for why they're doing what they do. It would be quite handy as part of the precommits I think, if at least as

Re: Add code quality checks to pre-commits.

+1 for adding more code quality signals. Could we add them in an advisory-only mode at first? (a warning and not an error) I'm curious how the "technical debt" metric is determined. I'm not familiar with SonarQube. What languages does it support? On Thu, Jan 3, 2019 at 10:19 AM Mikhail Gryzykhin

Add code quality checks to pre-commits.

Hi everyone, In our current builds we (can) run multiple code quality checks tools like checkstyle, findbugs, code test coverage via cubertura. However we do not utilize many of those signals. I suggest to add requirements to code based on those tools. Specifically, I suggest to add pre-commit ch