Static Analysis tests

Mon May 2 23:02:19 UTC 2016

On Mon, May 2, 2016 at 10:10 PM Nate Finch <nate.finch at canonical.com> wrote:

> I think it's a good point that we may only want to run some tests once,
> not once per OS/Arch.  However, I believe these particular tests are still
> limited in scope by the OS/arch of the host machine.  The go/build package
> respects the build tags in files, so in theory, one run of this on Ubuntu
> could miss code that is +build windows.  (we could give go/build different
> os/arch constraints, but then we're back to running this N times for N
> os/arch variants)
>
> I'm certainly happy to have these tests run in verify.sh, but it sounds
> like they may need to be run per-OS testrun as well.
>

We should investigate using go/build.Context.UseAllFiles to ignore the
build tags. As long as there are no "//+build ignore" files lurking, and as
long as we're not trying to do type-checking, that should be fine.

> On Sun, May 1, 2016 at 10:27 PM Andrew Wilkins <
> andrew.wilkins at canonical.com> wrote:
>
>> On Thu, Apr 28, 2016 at 11:48 AM Nate Finch <nate.finch at canonical.com>
>> wrote:
>>
>>> Maybe we're not as far apart as I thought at first.
>>>
>>
>>
>>> My thought was that they'd live under github.com/juju/juju/devrules (or
>>> some other name) and therefore only get run during a full test run or if
>>> you run them there specifically.  What is a full test run if not a test of
>>> all our code?  These tests just happen to test all the code at once, rather
>>> than piece by piece.  Combining with the other thread, if we also marked
>>> them as skipped under -short, you could easily still run go test ./...
>>> -short from the root of the juju repo and not incur the extra 16.5 seconds
>>> (gocheck has a nice feature where if you call c.Skip() in the SetUpSuite,
>>> it skips all the tests in the suite, which is particularly appropriate to
>>> these tests, since it's the SetUpSuite that takes all the time).
>>>
>>
>> I'm not opposed to using the Go testing framework in this instance,
>> because it makes most sense to write the analysis code in Go. That may not
>> always be the case, though, and I don't want to have a rule of "everything
>> as Go tests" that means we end up shoe-horning things. This is just
>> academic until we need something that doesn't live in the Go ecosystem,
>> though.
>>
>> Most importantly, I don't want to lose the ability to distinguish the
>> types of tests. As an example: where we run static analysis should never
>> matter, so we can cut a merge job short by performing all of the static
>> analysis checks up front. That doesn't matter much if we only gate merges
>> on running the tests on one Ubuntu series/arch; but what if we want to
>> start gating on Windows, CentOS, or additional architectures? It would not
>> make sense to run them all in parallel if they're all going to fail on the
>> static analysis tests. And then if we've run them up front, it would be
>> ideal to not have to run them on the individual test machines.
>>
>> So I think it would be OK to have a separate "devrules" package, or
>> whatever we want to call it. I would still like these tests to be run by
>> verify.sh, so we have one place to go to check that the source code is
>> healthy, without also running the unit tests or feature tests. If we have a
>> separate package like this, test tags are not really necessary in the short
>> term -- the distinction is made by separating the tests into their own
>> package. We could still mark them as short/long, but that's orthogonal to
>> separation-by-purpose.
>>
>> Cheers,
>> Andrew
>>
>> Mostly, I just didn't want them to live off in a separate repo or run
>>> with a separate tool.
>>>
>>> On Wed, Apr 27, 2016 at 11:39 PM Andrew Wilkins <
>>> andrew.wilkins at canonical.com> wrote:
>>>
>>>> On Thu, Apr 28, 2016 at 11:14 AM Nate Finch <nate.finch at canonical.com>
>>>> wrote:
>>>>
>>>>> From the other thread:
>>>>>
>>>>> I wrote a test that parses the entire codebase under
>>>>> github.com/juju/juju to look for places where we're creating a new
>>>>> value of crypto/tls.Config instead of using the new helper function that I
>>>>> wrote that creates one with more secure defaults.  It takes 16.5 seconds to
>>>>> run on my machine.  There's not really any getting around the fact that
>>>>> parsing the whole tree takes a long time.
>>>>>
>>>>> What I *don't* want is to put these tests somewhere else which
>>>>> requires more thought/setup to run.  So, no separate long-tests directory
>>>>> or anything.  Keep the tests close to the code and run in the same way we
>>>>> run unit tests.
>>>>>
>>>>
>>>> The general answer to this belongs back in the other thread, but I
>>>> agree that long-running *unit* tests (if there should ever be such a thing)
>>>> should not be shunted off to another location. Keep the unit tests with the
>>>> unit. Integration tests are a different matter, because they cross multiple
>>>> units. Likewise, tests for project policies.
>>>>
>>>> Andrew's response:
>>>>>
>>>>>
>>>>> *The nature of the test is important here: it's not a test of Juju
>>>>> functionality, but a test to ensure that we don't accidentally use a TLS
>>>>> configuration that doesn't match our project-wide constraints. It's static
>>>>> analysis, using the test framework; and FWIW, the sort of thing that Lingo
>>>>> would be a good fit for.*
>>>>>
>>>>> *I'd suggest that we do organise things like this separately, and run
>>>>> them as part of the "scripts/verify.sh" script. This is the sort of test
>>>>> that you shouldn't need to run often, but I'd like us to gate merges on.*
>>>>>
>>>>> So, I don't really think the method of testing should determine where
>>>>> a test lives or how it is run.  I could test the exact same things with a
>>>>> more common unit test - check the tls config we use when dialing the API is
>>>>> using tls 1.2, that it only uses these specific ciphersuites, etc.  In
>>>>> fact, we have some unit tests that do just that, to verify that SSL is
>>>>> disabled.  However, then we'd need to remember to write those same tests
>>>>> for every place we make a tls.Config.
>>>>>
>>>>
>>>> The method of testing is not particularly relevant; it's the *purpose*
>>>> that matters. You could probably use static analysis for a lot of our
>>>> units; it would be inappropriate, but they'd still be testing units, and so
>>>> should live with them.
>>>>
>>>> The point I was trying to make is that this is not a test of one unit,
>>>> but a policy that covers the entire codebase. You say that you don't want
>>>> to it put them "somewhere else", but it's not at all clear to me where you
>>>> think we *should* have them.
>>>>
>>>>> The thing I like about having this as part of the unit tests is that
>>>>> it's zero friction.  They already gate landings.  We can write them and run
>>>>> them them just like we write and run go tests 1000 times a day.  They're
>>>>> not special.  There's no other commands I need to remember to run, scripts
>>>>> I need to remember to set up.  It's go test, end of story.
>>>>>
>>>>
>>>> Using the Go testing framework is fine. I only want to make sure we're
>>>> not slowing down the edit/test cycle by frequently testing things that are
>>>> infrequently going to change. It's the same deal as with integration tests;
>>>> there's a trade-off between the time spent and confidence level.
>>>>
>>>>> The comment about Lingo is valid, though I think we have room for both
>>>>> in our processes.  Lingo, in my mind, is more appropriate at review-time,
>>>>> which allows us to write lingo rules that may not have 100% confidence.
>>>>> They can be strong suggestions rather than gating rules.  The type of test
>>>>> I wrote should be a gating rule - there are no false positives.
>>>>>
>>>>> To give a little more context, I wrote the test as a suite, where you
>>>>> can add tests to hook into the code parsing, so we can trivially add more
>>>>> tests that use the full parsed code, while only incurring the 16.5 second
>>>>> parsing hit once for the entire suite.  That doesn't really affect this
>>>>> discussion at all, but I figured people might appreciate that this could be
>>>>> extended for more than my one specific test.  I certainly wouldn't advocate
>>>>> people writing new 17 seconds tests all over the place.
>>>>>
>>>>
>>>> That sounds lovely, thank you.
>>>>
>>>> Cheers,
>>>> Andrew
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160502/f0f018b1/attachment-0001.html>