Improve developer experience for specs

[sdhull]

Description

When implementing a new feature I need to ensure my code works for every variation, but typically it's more important that I can verify the new variation works. But either way, just stubbing client.variation to return something doesn't exactly cut it because there could be many variations expecting a wide variety of return values.

Solution I'd like

First and foremost, simply add a "Testing" section to the Readme and/or official SDK docs.

If you wanted to help your customers even more, I think it would be nice to be able to specify something like this in my spec helper:

launch_darkly_client.variation_defaults = {
   "key" => true,
   "key2" => { something: :new },
}

(and these would override the default specified in ld_client.variation(key, user, default), because the default I pass in code is essentially "off" and the default I want in specs is typically "on"). Also I'm not sure about the method name, maybe it should be #variation_stubs or #override_variations or something like that.

But regardless of whether you implement that or not, simply adding a "Testing" section to the readme or documentation would go a long way to help the next person who wanders in wondering how the heck to wrangle LD in specs.

Alternatives considered

There is no alternative to good documentation.

[eli-darkly]

Thanks for bringing this up. We certainly want to make the developer experience as smooth as possible. However, I have some questions about what you wrote.

First, you mentioned the possibility of stubbing client.variation. Stubbing that method is exactly what we've assumed people would do in a test scenario like this; that's a common approach with libraries in general, so much so that it didn't occur to us to suggest it specifically in the docs.

However, you also said that stubbing that method would not be doable "because there could be many variations expecting a wide variety of return values". I'm guessing that you meant "there could be many feature flags, and the code would expect a different value for each flag" (in LaunchDarkly, "variation" doesn't mean a flag, it means one of those possible return values). And the variation_defaults mechanism that you proposed uses a hash to specify one value for each flag. But that kind of thing can easily be done with a mock/stub. For instance, if you're using RSpec:

  my_defaults = {
    "key" => true,
    "key2" => { something: :new },
  }
  client = double()
  client.stub(:variation) do | flag_key, user, default |
    values[flag_key]
  end

In fact, using a mock would be much more flexible than your proposed new feature because it could use any kind of logic you wanted, not just a key-value hash. For instance, in a specific test where only one flag is being evaluated, but you want it to return different values for two different users— and you also want to guarantee that no other unexpected parameters are being used in evaluations— you could do something like this:

  client.stub(:variation) do | flag_key, user, default |
    if flag_key == 'flag_under_test'
      return 'value1' if user[:key] == 'user1'
      return 'value2' if user[:key] == 'user2'
      raise ArgumentError, 'test specified an unexpected user'
    end
    raise ArgumentError, 'test specified an unexpected flag'
  end

So, unless I'm misunderstanding what you wrote, it doesn't seem to me that adding a client.variation_defaults property as you suggest would actually be helping our customers more, since it is possible to accomplish the same thing and more using a standard feature of the existing test framework— not specific to LaunchDarkly at all— which is meant for exactly this sort of scenario.

[sdhull]

Derp yeah that's a good suggestion.

I do believe it's worth adding a section around tests in your documentation, even if you just basically copy & paste the above. I believe folks in my org when down the path of emulating what is done in specs in this repo (eg upsert(LaunchDarkly::FEATURES, flag)), which is probably overkill for your clients. Because then I went down the rabbit-hole of trying to figure out what all the keys are in the flag hash and trying to figure out how to my feature flag simply return a specific variation. In doing so I learned way more about the ld library than I needed (or wanted) to know. And yes I got somewhat frazzled and used the wrong nomenclature. Apologies.

Basically if you can help steer folks away from doing that, it would be a win. I totally understand why you'd assume this wasn't necessary and I bet for most it's not, but just a little nudge in the right direction would likely go a long way.

[eli-darkly]

Sure, that's reasonable. I'm filing an internal issue to look over the online docs and see what we could do in that regard, although I'm not sure how much detail we would want to go into for things like stubbing that are implemented differently in different test frameworks. It's also possible that we'll end up adding some related functionality in the SDKs themselves, somewhat similar to the override feature you proposed, for cases where you might really want the rest of the SDK's functionality to behave exactly like it normally would and therefore wouldn't want to use a mock client— but that's still at the design stage.

Also, the main reason that the SDK's own tests use a more roundabout method is that they are testing the actual implementations of methods like variation, so it wouldn't make sense to stub out those methods— but, since the test code there has accumulated a bit of cruft over several years, it's likely that there are some places where a simpler approach would work and would be less confusing for anyone reading the test code.