Static Analysis tests

Mon May 2 23:03:11 UTC 2016

On Tue, May 3, 2016 at 2:47 AM John Meinel <john at arbash-meinel.com> wrote:

> Interestingly, putting it in "verify.sh" does have some fallout as well.
> At least on my machines verify is already slow enough to cause github to
> timeout while I'm pushing revisions. So I actually have to run it manually,
> and then git push --no-verify if I actually want it to complete. Adding 16s
> to that is a pretty serious overhead (I don't know the specific, but I
> think github times out at around 30s).
>

Agreed, it is getting a bit slow.
verify.sh -short? :)

John
> =:->
>
>
> On Mon, May 2, 2016 at 6: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.
>>
>> 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
>>>>>
>>>>
>> --
>> Juju-dev mailing list
>> Juju-dev at lists.ubuntu.com
>> Modify settings or unsubscribe at:
>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160502/bcb4e2b2/attachment.html>