{"action":"created","issue":{"url":"https://api.github.com/repos/NebulousLabs/Sia/issues/800","labels_url":"https://api.github.com/repos/NebulousLabs/Sia/issues/800/labels{/name}","comments_url":"https://api.github.com/repos/NebulousLabs/Sia/issues/800/comments","events_url":"https://api.github.com/repos/NebulousLabs/Sia/issues/800/events","html_url":"https://github.com/NebulousLabs/Sia/pull/800","id":108551283,"number":800,"title":"Refactors crypto/signatures.go to use explicit interfaces and adds unit tests","user":{"login":"mtlynch","id":7783288,"avatar_url":"https://avatars.githubusercontent.com/u/7783288?v=3","gravatar_id":"","url":"https://api.github.com/users/mtlynch","html_url":"https://github.com/mtlynch","followers_url":"https://api.github.com/users/mtlynch/followers","following_url":"https://api.github.com/users/mtlynch/following{/other_user}","gists_url":"https://api.github.com/users/mtlynch/gists{/gist_id}","starred_url":"https://api.github.com/users/mtlynch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mtlynch/subscriptions","organizations_url":"https://api.github.com/users/mtlynch/orgs","repos_url":"https://api.github.com/users/mtlynch/repos","events_url":"https://api.github.com/users/mtlynch/events{/privacy}","received_events_url":"https://api.github.com/users/mtlynch/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-09-27T18:07:06Z","updated_at":"2015-09-30T01:44:35Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/NebulousLabs/Sia/pulls/800","html_url":"https://github.com/NebulousLabs/Sia/pull/800","diff_url":"https://github.com/NebulousLabs/Sia/pull/800.diff","patch_url":"https://github.com/NebulousLabs/Sia/pull/800.patch"},"body":"Refactors crypto.GenerateSignatureKeys and crypto.DeterministicSignatureKeys\r\nso that they now depend on explicit interfaces.\r\n\r\nAdds unit tests to exercise the new Generate and GenerateDeterministic\r\nfunctions."},"comment":{"url":"https://api.github.com/repos/NebulousLabs/Sia/issues/comments/144252630","html_url":"https://github.com/NebulousLabs/Sia/pull/800#issuecomment-144252630","issue_url":"https://api.github.com/repos/NebulousLabs/Sia/issues/800","id":144252630,"user":{"login":"mtlynch","id":7783288,"avatar_url":"https://avatars.githubusercontent.com/u/7783288?v=3","gravatar_id":"","url":"https://api.github.com/users/mtlynch","html_url":"https://github.com/mtlynch","followers_url":"https://api.github.com/users/mtlynch/followers","following_url":"https://api.github.com/users/mtlynch/following{/other_user}","gists_url":"https://api.github.com/users/mtlynch/gists{/gist_id}","starred_url":"https://api.github.com/users/mtlynch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mtlynch/subscriptions","organizations_url":"https://api.github.com/users/mtlynch/orgs","repos_url":"https://api.github.com/users/mtlynch/repos","events_url":"https://api.github.com/users/mtlynch/events{/privacy}","received_events_url":"https://api.github.com/users/mtlynch/received_events","type":"User","site_admin":false},"created_at":"2015-09-30T01:44:35Z","updated_at":"2015-09-30T01:44:35Z","body":"Thanks for taking the time to review, @lukechampine!\r\n\r\n>I don't like their heavy use of reflection, variadic functions, etc. Particularly bad are the use of strings to specify function names in mock.On, and the use of opaque indices in expressions like args.Int(0)\r\n\r\nYeah, I agree with you that the guts are quite ugly. I'm coming off a lot of recent experience with Python's `mock` package which is *extremely* elegant, so seeing `testify/Mock` is depressing, though I think as a language Go is much more rigid and so it's much harder to fiddle around with internals to facilitate testing than it is in Python.\r\n\r\n>I can see the benefit of using mock to define the behavior of test objects. Essentially, it allows you to very purely test a function's logic without worrying about its dependencies. I'm just not sure it's worth the cost of a new framework. My gut says that you can replicate 90% of mock's functionality using simple stateful test objects. mock requires you to substitute interfaces for objects anyway, so after you do that, the mock package is really just for convenience. See upload_test.go and download_test.go for examples of where we're already using mocks.\r\n\r\nI don't know that we could do better rolling our own. What you're pointing to in upload_test.go and download_test.go seem to be test *fakes* rather than mocks. The difference is that test fakes are just a simpler, more testable implementation of the real object whereas mocks have programmable expectations (more detailed explanation [here](http://martinfowler.com/articles/mocksArentStubs.html)).\r\n\r\nI think in a lot of cases in this code, we want to test with mocks, not fakes. In upload_test, for example, it doesn't look like there's validation that `testHost.addPiece` was actually called, which might be fine in that case. But for something like `SignatureKeyGenerator.Generate` it's a pretty big problem if it generated keys without ever calling `rand.Read` because how could its generated keys be secure if it never accessed its source of entropy?\r\n\r\nA lot of times, mocks are valuable in making sure that the function was called in the right way. For example, when we have a function that makes a call to a signing interface, we want to verify not only that it calls that interface and properly handles a given result, but that it called that interface correctly (i.e. are we signing the correct data?).\r\n\r\nWe can implement our own mock features like creating a type that keeps a record of how its functions were called, but then we're basically reimplementing `testify/mock`.\r\n\r\n>I don't find testify tests more readable\r\n\r\nI think they're a vast improvement over manual equality checks and way more readable and easier to write. Every testing framework I've ever worked with has mocks that are some variant of `assert(expected, actual, message)` so maybe I'm just used to it, but Go's built-in testing seems primitive and crazy to me.\r\n\r\nWhy would you want to write this:\r\n\r\n```\r\nif (a != b) {\r\n  t.Errorf(\"Expected a==b, but a=%v, b=%v\", a, b)\r\n}\r\n```\r\n\r\nWhen you can just write:\r\n\r\n```\r\nassert.Equal(a, b)\r\n```\r\n\r\nI can live without testify's assert if you guys don't like it, but I think it's way better.\r\n\r\n>I dislike the separation of assert and require to delineate t.Error and t.Fatal; we frequently use both, which would mean creating both assert and require objects in many tests.\r\n\r\nI don't know if this makes a difference to you but creating the objects is just a convenience. You can also call like this:\r\n\r\n```\r\nassert.Equals(t, a, b)\r\nrequire.Equals(t, a, b)\r\n```\r\n\r\n>Reliance on Equal also seems dangerous, especially when dealing with structs and pointers. Better to be explicit about what you are comparing.\r\n\r\nI'm not clear on what you mean here. Aren't the following two snippets equally precise about what they're comparing?\r\n\r\n```\r\nif (a != b) { t.Error(\"a!=b\") }\r\n```\r\n```\r\nassert.Equal(a, b)\r\n```\r\n\r\n>The SignatureKeyGenerator type looks strange. It's a struct, but its fields are both functions. That sounds like an interface to me!\r\n\r\n@DavidVorick said the same thing, but then I think it made sense for him after he spent some time with it. It's a struct whose fields are function *pointers* which is different from an interface.\r\n\r\n> You don't need to define functions like NewTestableSignatureKeyGenerator to initialize a struct; just use a literal.\r\n\r\nTrue, fixed in c666d24.\r\n\r\n> Prefer object{} to *new(object).\r\n\r\nFixed in 226ade0.\r\n\r\n>The point of all this refactoring and new code, as far as I can tell, is to break up the tight coupling of the crypto functions and their source of randomness (currently crypto/rand).\r\n\r\nThis is exactly correct.\r\n\r\n>This would allow us to test code paths where the crypto functions fail.\r\n\r\nCorrect, but only for the code where we successfully break up the tight coupling. In this case, it allows us to unit test code paths within `SignatureKeyGenerator` but it doesn't improve testability of `SignatureKeyGenerator`'s clients much.\r\n\r\n> in this PR you modified modules/host/host.go to use your new skg object. But there's still no way to make Generate fail in the host tests!\r\n\r\nCorrect, you can't make `Generate` fail from modules/host/host.go, but that's beyond the scope of this PR as it's a separate problem that `host.go` is tightly coupled with `SignatureKeyGenerator.Generate` (rather than a dependency on an interface, like `KeyPairGenerator`).\r\n\r\n> I just worry that there's a slippery slope here, where we wind up mocking anything that involves I/O, including the entire filesystem (there are actually multiple packages that do this, e.g. https://github.com/spf13/afero).\r\n\r\nI don't want to mock anything that involves I/O. I want to mock **[everything](https://www.youtube.com/watch?v=KBKXu3Kg4yg)**! *Especially* the filesystem.\r\n\r\nIt's not as bad as you think...\r\n\r\n> we use crypto functions all over the place, and sometimes they are buried quite deeply in the call stack.\r\n\r\nWe only mock out a unit's immediate dependencies, so if it's more than 1 level deep, we don't have to worry about it.\r\n\r\nLet's take the example of host.go. Our production code might look like:\r\n\r\n```\r\nhost.go : New()\r\n -> SignatureKeyGenerator.Generate (via KeyPairGenerator interface)\r\n   -> rand.Read (via readBytesFunc)\r\n```\r\n\r\nBut our test would just look like:\r\n\r\n```\r\nhost.go : New()\r\n -> MockSignatureKeyGenerator.Generate (via KeyPairGenerator interface)\r\n```\r\n\r\nThis is the benefit of loose coupling. Each class communicates with its dependencies through well defined interfaces and the lower level implementation details of those dependencies don't matter.\r\n\r\nBack to filesystems: We *really* want to mock out filesystem calls. The filesystem is shared global state across tests so it's very easy for tests to taint one another via the filesystem (i.e. generate false positives or negatives). Plus it generally makes the test loop much slower."}}