|
|
Descriptionworker/notifyworker.go: Common worker logic
While I was looking to implement Upgrader as a worker, I realized that there
was a lot of bits necessary to implement a Worker. You have to integrate with a
tomb, and monitor the worker, and handle a modest interface, etc. It isn't a
terrible amount, but it turns out to be a lot of boilerplate that actually
takes a lot if you want to test all the edge cases.
This patch pulls all of that edge-case logic into a NotifyWorker helper.
Essentially you implement a smaller api (WatchHandler), and hand that to the
NotifyWorker. It then handles all the logic about shutting down cleanly, and
you just integrate your couple of actions based on triggers.
I have a follow up patch so you can see what the Cleaner worker looks like with
this change.
Probably the biggest advantage of this patch is that I have a very well tested
edge-case-handling code, which can now just be re-used for all the other
workers, and you don't have to worry about if your code is stopping the watcher
it started, or if it is shutting down at the right time, etc.
https://code.launchpad.net/~jameinel/juju-core/notify-worker/+merge/173354
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 23
Patch Set 2 : worker/notifywatcher.go: Common worker logic #
Total comments: 24
Patch Set 3 : worker/notifywatcher.go: Common worker logic #
Total comments: 13
Patch Set 4 : worker/notifyworker.go: Common worker logic #
MessagesTotal messages: 12
Please take a look.
Sign in to reply to this message.
On 2013/07/07 14:02:36, jameinel wrote: > Please take a look. See https://codereview.appspot.com/10876048/ for an example of how this can simplify a worker implementation.
Sign in to reply to this message.
https://codereview.appspot.com/10978043/diff/1/worker/export_test.go File worker/export_test.go (right): https://codereview.appspot.com/10978043/diff/1/worker/export_test.go#newcode19 worker/export_test.go:19: old := nw.closedHandler this will probably cause the race detector to have a whinge. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/1/worker/notifyworker.go#newcode33 worker/notifyworker.go:33: String() string drop this from the interface, then you don't need to implement it in your mock. fmt will always detect anything that matches fmt.Stringer and call its .String() method anyway. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker.go#newcode94 worker/notifyworker.go:94: } could you do this check in NewNotifyWorker ? https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go File worker/notifyworker_test.go (right): https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:52: mu sync.Mutex /s/mu// then you can just do a.Lock() etc https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:82: a.handled <- struct{}{} a.handled is not buffered, is there possibility of a deadlock here if something else needs a.mu.Lock? https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:98: // Stop never returns can the test suite reliably continue if the worker has not stopped ? https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:111: c.Errorf("Failed to stop worker after %.3fs", longWait.Seconds()) c.Errorf("Failed to stop worker after %s", longWait) // time.Durations know how to print themselves. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:118: mu sync.Mutex same as above. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:154: c.Errorf("Timed changes triggering change after %.3fs", longWait.Seconds()) same https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:167: c.Errorf("Wait() failed to return after %.3fs", shortWait.Seconds()) same. This block is repeated a few times, how about a helper, that takes a c.C, a <-chan and a timeout ? https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:177: c.Errorf("handled failed to signal after", longWait.Seconds()) missing format specifier, %s or %v, same as above. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:202: c.Errorf("Wait() didn't wait until we stopped it. err: %v", err) s/err:/:/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/10978043/diff/1/worker/export_test.go File worker/export_test.go (right): https://codereview.appspot.com/10978043/diff/1/worker/export_test.go#newcode19 worker/export_test.go:19: old := nw.closedHandler On 2013/07/08 02:46:07, dfc wrote: > this will probably cause the race detector to have a whinge. On the reader side, closedHandler is only accessed after a channel has been detected as closed (which is a sync event, isn't it?). And that close() happens after SetClosedHandler was called in response to calling Stop() after SetClosedHandler. Is that sufficient for the race detector? I could add a mutex, but it seems silly to do a mutex check on read access when the only thing that can change it is test code. I don't have easy access to the race detector, so if you want to check and let me know if it needs to be changed. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/1/worker/notifyworker.go#newcode33 worker/notifyworker.go:33: String() string On 2013/07/08 02:46:07, dfc wrote: > drop this from the interface, then you don't need to implement it in your mock. > fmt will always detect anything that matches fmt.Stringer and call its .String() > method anyway. I wanted to have the test "TestStringForwardsHandlerString" which asserts that NotifyWorker gets its format string from the underlying WatchHandler. I could do that with either: fmt.Sprintf(s.worker) or s.worker.(fmt.Stringer).String() I guess. Though Workers all implement it currently, and we sort of want to force them to implement it so we get nicer error and logging messages, right? I'll take it out per your request, but it doesn't seem harmful to require it. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker.go#newcode94 worker/notifyworker.go:94: } On 2013/07/08 02:46:07, dfc wrote: > could you do this check in NewNotifyWorker ? It could be done that way. Though all of the existing workers that we have follow the pattern that New* doesn't validate and have the possibility of an error, as those are found later. Specifically I checked: NewDeployer, NewFirewaller, NewCleaner, NewMachiner, NewProvisioner, etc. It may be just that I'm more rigorous about testing things like passing nil (all of those will fail with nil pointer dereference panics if you pass State = nil, for example). NotifyWorker itself cares about its handler in String() which is why I cared a bit more (getting a null pointer dereference while trying to log something else is failing is particularly crummy). I suppose I could panic() in NewDeployer, so we at least fail early rather than at a random later time. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go File worker/notifyworker_test.go (right): https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:52: mu sync.Mutex On 2013/07/08 02:46:07, dfc wrote: > /s/mu// then you can just do a.Lock() etc I know that is possible, but I've always found it more confusing than helpful. If you have a Lock attribute, where did it come from? You have to know ahead of time all of the types that are embedded in the master type and what methods they might add. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:82: a.handled <- struct{}{} On 2013/07/08 02:46:07, dfc wrote: > a.handled is not buffered, is there possibility of a deadlock here if something > else needs a.mu.Lock? It doesn't happen in how the code is explicitly exercised today, but it is potentially a problem. It is fine for handled to be buffered. The tests care that Handle has been triggered before we assert, but they don't have to happen in lock-step. I Unlock and buffered the handled channel. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:98: // Stop never returns On 2013/07/08 02:46:07, dfc wrote: > can the test suite reliably continue if the worker has not stopped ? If we block, then the test suite just hangs for 600s until the test runner kills it, and we don't get any useful information. If the test suite fails, at least it fails reasonably quickly and gives locality to where the failure is. You *can* run the rest of the tests even if you have extra stuff lying around (it isn't as clean, and I'd rather avoid having tests doing that, which is why the test will still fail). https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:111: c.Errorf("Failed to stop worker after %.3fs", longWait.Seconds()) On 2013/07/08 02:46:07, dfc wrote: > c.Errorf("Failed to stop worker after %s", longWait) // time.Durations know how > to print themselves. Done. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:154: c.Errorf("Timed changes triggering change after %.3fs", longWait.Seconds()) On 2013/07/08 02:46:07, dfc wrote: > same Done. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:167: c.Errorf("Wait() failed to return after %.3fs", shortWait.Seconds()) On 2013/07/08 02:46:07, dfc wrote: > same. This block is repeated a few times, how about a helper, that takes a c.C, > a <-chan and a timeout ? I pulled out a few that were using a <- chan error We have another one that uses a <-chan struct{}, and another one that has inverted logic. (We expect to timeout, rather than expecting to complete.) But it did reduce the number of them. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:177: c.Errorf("handled failed to signal after", longWait.Seconds()) On 2013/07/08 02:46:07, dfc wrote: > missing format specifier, %s or %v, same as above. Done. https://codereview.appspot.com/10978043/diff/1/worker/notifyworker_test.go#ne... worker/notifyworker_test.go:202: c.Errorf("Wait() didn't wait until we stopped it. err: %v", err) On 2013/07/08 02:46:07, dfc wrote: > s/err:/:/ Done.
Sign in to reply to this message.
Very good approach, but would like some changes. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:30: Wait() error Would like standard commenting here. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:38: type WatchHandler interface { SetUp() and TearDown() remind of tests and so easily can lead to confusion. How about Init() and Terminate()? https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:52: func NewNotifyWorker(handler WatchHandler) NotifyWorker { Comment. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:53: if handler == nil { Hmm, so far we don't do kinds of asserts to check preconditions (nil, ranges, empty strings, etc.). In case the developer passes nil here it has to crash during the tests. This doesn't mean I've problems with doing pres and posts like in Eiffel. But it's uncommon here. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:72: // Kill and Wait for this to exit // Stop kills the worker and waits for it to exit. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:83: // Return a nice description of this worker // String returns ... https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:87: // to debug. Sure, a very defensive way. But then I would like a package for it: - precond.NotNil(foo, "foo must not be nil") - precond.Range(foo, 0, 100, "foo must be between 0 and 100") - ...
Sign in to reply to this message.
Very nice. LGTM (although some of mue's comments should be handled too) https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:38: type WatchHandler interface { On 2013/07/08 09:22:39, mue wrote: > SetUp() and TearDown() remind of tests and so easily can lead to confusion. How > about Init() and Terminate()? Please no. SetUp and TearDown mean the same things here and in tests. Why force people to learn new terminology? https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:44: TearDown() TearDown ought to be able to error, I think. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:87: // to debug. On 2013/07/08 09:22:39, mue wrote: > Sure, a very defensive way. But then I would like a package for it: > > - precond.NotNil(foo, "foo must not be nil") > - precond.Range(foo, 0, 100, "foo must be between 0 and 100") > - ... I think you may have misunderstood jam's intent here. I don't see anything here that justifies a precond package. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:95: // Replace calls to TearDown with a defer nw.handler.TearDown() d https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:109: } Is this worth the noise? Shouldn't we just panic as we do implicitly whenever some halfwit starts flinging nils around? ;) https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker_test.go File worker/notifyworker_test.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker_test.go... worker/notifyworker_test.go:140: tw.changes = nil d (a closed channel and a nil channel behave very differently)
Sign in to reply to this message.
https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:87: // to debug. On 2013/07/08 09:53:49, fwereade wrote: > On 2013/07/08 09:22:39, mue wrote: > > Sure, a very defensive way. But then I would like a package for it: > > > > - precond.NotNil(foo, "foo must not be nil") > > - precond.Range(foo, 0, 100, "foo must be between 0 and 100") > > - ... > > I think you may have misunderstood jam's intent here. I don't see anything here > that justifies a precond package. Hehe, no, here you got me wrong. I don't want this package. It only has been a kind of loud thinking where a more defensive programming like in Eiffel would lead to. We've got more places where a passed nil would lead to a panic.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:38: type WatchHandler interface { On 2013/07/08 09:22:39, mue wrote: > SetUp() and TearDown() remind of tests and so easily can lead to confusion. How > about Init() and Terminate()? Done. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:44: TearDown() On 2013/07/08 09:53:49, fwereade wrote: > TearDown ought to be able to error, I think. Certainly I initially wrote it returning an error, but found that everywhere that was calling TearDown was unable to do anything with that error because it was already dealing with the current error. Since the logic is going to end up "Log an error if it occurs during cleanup" I'd be happier to have people writing the cleanup function realize that their errors are going to be ignored. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:53: if handler == nil { On 2013/07/08 09:22:39, mue wrote: > Hmm, so far we don't do kinds of asserts to check preconditions (nil, ranges, > empty strings, etc.). In case the developer passes nil here it has to crash > during the tests. > > This doesn't mean I've problems with doing pres and posts like in Eiffel. But > it's uncommon here. I can say it is uncommon to see explicit testing of parameters. I can also say that the test coverage isn't quite what I'm used to. Specifically, if you have an error condition that isn't properly exposed, you can end up where a failure in one part ends up passing a nil to another part, which ends up failing in some random place like String() and ends up making it hard to track down the original error. I can understand detecting things in test cases, but integration tests rarely cover the myriad of failure cases (because they tend to be hard to set up rather than unit tests that poke bad state in directly). Anyway, I'll follow the crowd and not directly provoke this. Without this check you end up getting a panic in the loop goroutine at "some asynchronous point in the future", so it can be a bit hard to know *who* passed in the nil. Perhaps I'm being overly cautious. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:72: // Kill and Wait for this to exit On 2013/07/08 09:22:39, mue wrote: > // Stop kills the worker and waits for it to exit. Done. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:87: // to debug. On 2013/07/08 09:22:39, mue wrote: > Sure, a very defensive way. But then I would like a package for it: > > - precond.NotNil(foo, "foo must not be nil") > - precond.Range(foo, 0, 100, "foo must be between 0 and 100") > - ... If you take something from outside, and you rely on that state inside, it feels like something you should validate. (Public vs private APIs and all). Given it isn't convention, I'll back down. The fact that it fails late (in a different goroutine) really bothers me, though. Because the thing that passed in the bad state is long gone from the traceback by the time you have noticed it. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:95: // Replace calls to TearDown with a defer nw.handler.TearDown() On 2013/07/08 09:53:49, fwereade wrote: > d Done. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:109: } On 2013/07/08 09:53:49, fwereade wrote: > Is this worth the noise? Shouldn't we just panic as we do implicitly whenever > some halfwit starts flinging nils around? ;) The problem is that panics are happening in a goroutine. I was doing this in the test suite (while building up proper code), which meant I ended up with giant panics that couldn't be trapped. So while I agree that a handler that returns 'nil' for its watcher is badly coded, it is far easier to get a test failure that says "duuude, you returned a nil without an error, man". For example, I can write the nice clean "TestSetupNilWatcherStopsWithTeardown" test if I return an error here. I can't write a test case for the panic() version, because it is a panic in a goroutine, and not in the local context. (I can't call c.Assert(, PanicMatches) because the panic isn't in the stack.) Anyway, I can remove the test case, and the assert that if something is poorly coded we transition gracefully into an error state. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker_test.go File worker/notifyworker_test.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker_test.go... worker/notifyworker_test.go:140: tw.changes = nil On 2013/07/08 09:53:49, fwereade wrote: > d > > (a closed channel and a nil channel behave very differently) Calling close() twice on a channel causes a panic. Which means that calling Stop() on a Stoped Watcher is a panic rather than a no-op. I suppose I can just check tw.stopped.
Sign in to reply to this message.
I'd generally be hoping that whatever might return a nil watcher would itself be unit tested sufficiently that we'd be able to detect the problems from the tests for that component, but I follow your reasoning. Still LGTM apart from the error-swallowing. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:44: TearDown() On 2013/07/08 10:17:36, jameinel wrote: > On 2013/07/08 09:53:49, fwereade wrote: > > TearDown ought to be able to error, I think. > > Certainly I initially wrote it returning an error, but found that everywhere > that was calling TearDown was unable to do anything with that error because it > was already dealing with the current error. > > Since the logic is going to end up "Log an error if it occurs during cleanup" > I'd be happier to have people writing the cleanup function realize that their > errors are going to be ignored. The killing error might easily be nil. If we're going to just drop these on the floor we may as well not bother using a tomb at all.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Nice, LGTM with mostly trivial suggestions. There is some inconsistency with exported/unexported helpers in the test module, ideally we should use "waitX" "waitY" and the suites names as well with lowercase first. https://codereview.appspot.com/10978043/diff/15001/worker/export_test.go File worker/export_test.go (right): https://codereview.appspot.com/10978043/diff/15001/worker/export_test.go#newc... worker/export_test.go:7: "launchpad.net/juju-core/state/watcher" i'd put this on a single line, not as a block, but as you wish. https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:15: // Internal structure please put empty lines between the pairs of comment + field for better readability https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:43: d? https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:47: SetUp() (state.NotifyWatcher, error) // SetUp starts the handler... https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:49: // Cleanup any resources that are left around // TearDown cleans up ... https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:52: // The Watcher has indicated there are changes, do whatever work is // Handle is called when the Watcher ... https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:56: // Report a nice string identifying this worker // String returns a nice ... https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.go File worker/notifyworker_test.go (right): https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g... worker/notifyworker_test.go:13: not getting the separation here https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g... worker/notifyworker_test.go:21: var shortWait = 5 * time.Millisecond please comment which is used for what https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g... worker/notifyworker_test.go:50: type ActionsHandler struct { why exported?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/8001/worker/notifyworker.go#newc... worker/notifyworker.go:44: TearDown() On 2013/07/08 10:24:08, fwereade wrote: > On 2013/07/08 10:17:36, jameinel wrote: > > On 2013/07/08 09:53:49, fwereade wrote: > > > TearDown ought to be able to error, I think. > > > > Certainly I initially wrote it returning an error, but found that everywhere > > that was calling TearDown was unable to do anything with that error because it > > was already dealing with the current error. > > > > Since the logic is going to end up "Log an error if it occurs during cleanup" > > I'd be happier to have people writing the cleanup function realize that their > > errors are going to be ignored. > > The killing error might easily be nil. If we're going to just drop these on the > floor we may as well not bother using a tomb at all. Done. https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go File worker/notifyworker.go (right): https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker.go#new... worker/notifyworker.go:43: On 2013/07/08 10:51:04, dimitern wrote: > d? If you want spaces between them *I* find it easier to read each one with a blank line before it. That way the block which is the description of the type is by itself, and each block which describes the attribute is by itself. I don't quite understand why the first block would be special and attached to the type line. https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.go File worker/notifyworker_test.go (right): https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g... worker/notifyworker_test.go:13: On 2013/07/08 10:51:04, dimitern wrote: > not getting the separation here standard library imports (time sync fmt net/http, etc.) 3rd party imports (gocheck tomb mgo) juju-core imports (juju-core/state, juju-core/worker, etc.) https://codereview.appspot.com/10978043/diff/15001/worker/notifyworker_test.g... worker/notifyworker_test.go:50: type ActionsHandler struct { On 2013/07/08 10:51:04, dimitern wrote: > why exported? It doesn't need to be, but *its a test suite* nothing can import it anyway. I don't find the hiding of things to be particularly helpful in test suites, and I still haven't gotten over "CamelCase" for ClassNames. Privatized.
Sign in to reply to this message.
|