CUS-377: oauthtoken: refactor to single fake LoadStorer implementation

[jayconrod]

Preparing for a change where I'll add a Delete method to the
interface. We had 5 implementations of LoadStorer. Now we have 4.

• Removed File. It's no longer used in production.
• Added FakeLoadStorer. This replaces engflow_auth.fakeStore and File
  in tests.
• Simplified keyring test to call keyring.MockInit() instead of using
  our own shims.

[jayconrod]

@minor-fixes Gentle ping for review

[minor-fixes]

Do you think it's worth naming this fake_test.go so it ends up effectively testonly?

Alternatively, if we want this in the binary - it could be named InMemoryStore or similar, since it seems like its a legitimate implementation

Side question on naming - I've always struggled with Go interface vs. struct naming convention (suffixing all interfaces with -er is sometimes limiting/awkward, avoiding -er for struct types is also sometimes limiting, so I've historically follow the conventions sporadically depending on my mood). I notice here you've chosen FakeLoadStorer for the struct type name - what's your approach to naming structs/interfaces?

[jayconrod]

Do you think it's worth naming this fake_test.go so it ends up effectively testonly?

Alternatively, if we want this in the binary - it could be named InMemoryStore or similar, since it seems like its a legitimate implementation

I would if this were a single package, but this is needed in cmd/engflow_auth/main_test.go, so it needs to be exported from the library part of the package. I think that's okay: libraries sometimes provide fake implementations for other folks to test against. keyring itself does this.

Side question on naming - I've always struggled with Go interface vs. struct naming convention (suffixing all interfaces with -er is sometimes limiting/awkward, avoiding -er for struct types is also sometimes limiting, so I've historically follow the conventions sporadically depending on my mood). I notice here you've chosen FakeLoadStorer for the struct type name - what's your approach to naming structs/interfaces?

Renamed to FakeTokenStore. Actually, I think LoadStorer should be renamed to TokenStore, though I haven't done so in this PR. Let me know if I should.

I think the advice to name interfaces after verbs only works if the interface has one or two methods, and it's really generic, like io.ReadCloser. An io.ReadCloser could be basically anything, so it's hard to come up with a more descriptive name.

If something has a specific purpose, it's better to name it after that. For example, io/fs.FS has one method, Open(string) (File, error). FS is a much better name than Opener, and File is a much better name than StatReadCloser.

[minor-fixes]

Actually, I think LoadStorer should be renamed to TokenStore

Agree 100% - I avoided naming it this since it didn't end in er, and I was making an extra effort not to deviate from convention. Makes sense about the fs.FS naming - I guess the convention wasn't as strong as I had figured

[minor-fixes]

This test used to prove that we correctly handle calls to package-level functions in the keyring package, which our code treats as a black box - we were able to test what happens in our code when keyring.Get or keyring.Set fails. It seems like this change reduces coverage here - what's the rationale?

[jayconrod]

I've added a couple additional tests to restore coverage, see what you think.

I think the previous test was too artificial. We weren't doing much more than calling our own mock. I also don't think there's much value in tracking the call count. The library provides its own mocking functionality, so it seems better to use that and call the real functions, mocked internally.

[minor-fixes]

It was a bit artificial; If the keyring package had some way to list secrets as well, it might have gotten more interesting (a 0-arg logout might list all secrets, and delete them one by one - in which case the test could have mocked List to return N secrets, and then expect N Delete calls) - but with the current functionality, I like the simplification

[jayconrod]

It's weird that they provide a Keyring type but no way to get an instance of it. I guess it can be used for mocks, but we could just define our own type (which we previously did).

[jayconrod]

@minor-fixes Mind approving again? I pushed another commit to resolve a merge error, but it seems to invalidate the approval.