<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, May 3, 2016 at 2:47 AM John Meinel <<a href="mailto:john@arbash-meinel.com">john@arbash-meinel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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).</div></blockquote><div><br></div><div>Agreed, it is getting a bit slow.</div><div>verify.sh -short? :)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>John</div><div>=:-><br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"></div></div><div class="gmail_extra"><div class="gmail_quote">On Mon, May 2, 2016 at 6:10 PM, Nate Finch <span dir="ltr"><<a href="mailto:nate.finch@canonical.com" target="_blank">nate.finch@canonical.com</a>></span> wrote:<br></div></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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)<div><br></div><div>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. </div></div><div><div><br><div class="gmail_quote"><div dir="ltr">On Sun, May 1, 2016 at 10:27 PM Andrew Wilkins <<a href="mailto:andrew.wilkins@canonical.com" target="_blank">andrew.wilkins@canonical.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Apr 28, 2016 at 11:48 AM Nate Finch <<a href="mailto:nate.finch@canonical.com" target="_blank">nate.finch@canonical.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Maybe we're not as far apart as I thought at first. </div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>My thought was that they'd live under <a href="http://github.com/juju/juju/devrules" target="_blank">github.com/juju/juju/devrules</a> (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).</div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div><span style="line-height:1.5">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.</span><br></div><div><br></div><div>Most importantly, I don't want to lose the ability to distinguish the types of tests. As an example: w<span style="line-height:1.5">here 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.</span></div><div><span style="line-height:1.5"><br></span></div><div>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.</div><div><br></div><div>Cheers,</div><div>Andrew</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Mostly, I just didn't want them to live off in a separate repo or run with a separate tool.</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Apr 27, 2016 at 11:39 PM Andrew Wilkins <<a href="mailto:andrew.wilkins@canonical.com" target="_blank">andrew.wilkins@canonical.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Apr 28, 2016 at 11:14 AM Nate Finch <<a href="mailto:nate.finch@canonical.com" target="_blank">nate.finch@canonical.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">From the other thread:<div><br></div><div><span style="color:rgb(33,33,33);font-size:13px">I wrote a test that parses the entire codebase under </span><a href="http://github.com/juju/juju" style="font-size:13px" target="_blank">github.com/juju/juju</a><span style="color:rgb(33,33,33);font-size:13px"> 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.</span><br></div><div><span style="color:rgb(33,33,33);font-size:13px"><br></span></div><div><span style="color:rgb(33,33,33);font-size:13px">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.</span></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>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.</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><span style="color:rgb(33,33,33);font-size:13px">Andrew's response: </span></div><div><p style="color:rgb(34,34,34);font-family:Helvetica,Arial,sans-serif;font-size:14px;line-height:19px;margin-top:0px!important"><i><br>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></p><p style="color:rgb(34,34,34);font-family:Helvetica,Arial,sans-serif;font-size:14px;line-height:19px;margin-bottom:0px!important"><i>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.</i></p><p style="color:rgb(34,34,34);font-family:Helvetica,Arial,sans-serif;font-size:14px;line-height:19px;margin-bottom:0px!important">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.</p></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div><span style="line-height:1.5">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.<br></span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">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.</span></div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><p style="color:rgb(34,34,34);font-family:Helvetica,Arial,sans-serif;font-size:14px;line-height:19px;margin-bottom:0px!important">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.</p></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>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.</div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><p style="color:rgb(34,34,34);font-family:Helvetica,Arial,sans-serif;font-size:14px;line-height:19px;margin-bottom:0px!important">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.</p><p style="color:rgb(34,34,34);font-family:Helvetica,Arial,sans-serif;font-size:14px;line-height:19px;margin-bottom:0px!important">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.<br></p></div></div></blockquote><div><span style="line-height:1.5"><br></span></div></div></div><div dir="ltr"><div class="gmail_quote"><div><span style="line-height:1.5">That sounds lovely, thank you.</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">Cheers,</span></div><div><span style="line-height:1.5">Andrew</span></div></div></div> </blockquote></div> </blockquote></div></div></blockquote></div> </div></div><br></blockquote></div></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">--<br> Juju-dev mailing list<br> <a href="mailto:Juju-dev@lists.ubuntu.com" target="_blank">Juju-dev@lists.ubuntu.com</a><br> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" rel="noreferrer" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a><br> <br></blockquote></div></div></blockquote></div></div>