On Tue, May 3, 2016 at 2:47 AM John Meinel <[email protected]> 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 <[email protected]> > 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 < >> [email protected]> wrote: >> >>> On Thu, Apr 28, 2016 at 11:48 AM Nate Finch <[email protected]> >>> 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 < >>>> [email protected]> wrote: >>>> >>>>> On Thu, Apr 28, 2016 at 11:14 AM Nate Finch <[email protected]> >>>>> 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 >> [email protected] >> Modify settings or unsubscribe at: >> https://lists.ubuntu.com/mailman/listinfo/juju-dev >> >>
-- Juju-dev mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
