Detect custom OpenAPI spec file and start stripe-mock from test suite

[ob-stripe]

r? @brandur-stripe @remi-stripe

First attempt at embedding OpenAPI spec and fixtures files directly in the repo and using those for the test suite. Pretty rough, but it works.

Possible improvements:

• better logs / error handling (done)
• automatically choose an unused port (done)
• detect "Listening for HTTP on port" in stripe-mock output instead of sleeping for 1 second

[ob-stripe]

And of course the subprocess gem doesn't work with JRuby :(

[ob-stripe]

Alright, went for a slightly different approach.

The test suite now checks if STRIPE_MOCK_PORT is set. If it is, it assumes that stripe-mock is already running and uses that port, otherwise (if not running on JRuby) it attempts to start its own stripe-mock process on a random unused port itself (and kills it once it's done).

The idea is that on CI, we'd manually start stripe-mock and set STRIPE_MOCK_PORT and on our development machines we can simply run the test suite and it will spawn the process itself.

Let me know what you think! If you think this is a good approach, I'll update the README file accordingly.

[brandur-stripe]

Just wanted to make sure, but I assume that this is an artifact of an older attempt? It seems to be running basically the same thing as Stripe::StripeMock.

[brandur-stripe]

Ah, I saw your note about running differently in CI. Still not sure I 100% understand the expected workflow here, but I'll leave that in a comment below.

[brandur-stripe]

@ob-stripe Thanks for taking a stab here! Sorry about the super late reply :/

First of all: I just want to get a general idea of the suggested workflow here. Is the idea that test/stripe_mock.rb would come into master, but most of these pieces like changes to .travis.yml and test/openapi/ would only be present on development branches where they're needed?

If so, I wonder if we should just have a single override flag somewhere for use in CI that boots an automatic stripe-mock (like you get locally), but requires minimal changes to .travis.yml or anything else just to make the workflow as easy as possible.

And sorry, but just a couple other points of FUD for completeness:

1. If we can avoid it, I'd prefer not to bring any OpenAPI specs into the master tree because they'll bloat the repository quite a bit. The spec is ~1.5 MB while all other files in the repo combined are ~1 MB.
2. IMO if we go this route, we should avoid the use of the subprocess gem, and prefer just the use of the standard library's subprocess-handling mechanics, which are more than sufficient. As far as I can tell, subprocess is a Stripe NIH project that was probably developed originally due to insufficient understanding of built-in mechanics (see issue logged here years ago). It's not maintained, and not worth the new dependency.

[ob-stripe]

Is the idea that test/stripe_mock.rb would come into master, but most of these pieces like changes to .travis.yml and test/openapi/ would only be present on development branches where they're needed?

I don't feel super strongly about it but I'd personally lean towards having everything in master, for consistency and to make it easier to deal with multiple betas in parallel. It would also make it easier to identify the exact surface of a specific beta, by looking at the diff for the spec.

If we can avoid it, I'd prefer not to bring any OpenAPI specs into the master tree because they'll bloat the repository quite a bit. The spec is ~1.5 MB while all other files in the repo combined are ~1 MB.

Good point. I just copied the raw files for simplicity's sake, but we could also use a git submodule like in stripe-mock.

IMO if we go this route, we should avoid the use of the subprocess gem, and prefer just the use of the standard library's subprocess-handling mechanics, which are more than sufficient. As far as I can tell, subprocess is a Stripe NIH project that was probably developed originally due to insufficient understanding of built-in mechanics (see issue logged here years ago). It's not maintained, and not worth the new dependency.

👍 I've updated the PR to remove the dependency on subprocess and use ::Process.spawn instead. The good news is that this works with JRuby, so it's no longer necessary to start stripe-mock externally even in CI. I've updated .travis.yml to simply unpack stripe-mock and add it to PATH.

[brandur-stripe]

I don't feel super strongly about it but I'd personally lean towards having everything in master, for consistency and to make it easier to deal with multiple betas in parallel. It would also make it easier to identify the exact surface of a specific beta, by looking at the diff for the spec.

Oh I see. So the current (non-alpha/beta/etc.) OpenAPI spec is in master, and branches where alphas/betas/etc are implemented just overwrite that spec right?

Good point. I just copied the raw files for simplicity's sake, but we could also use a git submodule like in stripe-mock.

K cool. Not saying we should do that for sure because as discussed before, submodules are a pain, but I'm a little afraid that we eventually go through enough iterations on the spec that we end up accidentally making the repo pretty heavyweight.

👍 I've updated the PR to remove the dependency on subprocess and use ::Process.spawn instead. The good news is that this works with JRuby, so it's no longer necessary to start stripe-mock externally even in CI. I've updated .travis.yml to simply unpack stripe-mock and add it to PATH.

Awesome! This is wayyyy better.

Just reading this over, it looks to me that when running the test suite locally, a new version of stripe-mock is always started, even on the master branch (unless you have STRIPE_MOCK_PORT set in your environment, but I don't think most people do). Was that intentional? It seems like we should try to have it favor the already-running stripe-mock where possible for speed if nothing else.

[ob-stripe]

Oh I see. So the current (non-alpha/beta/etc.) OpenAPI spec is in master, and branches where alphas/betas/etc are implemented just overwrite that spec right?

Yeah exactly. This way you'd have all the information in one place when looking at the diff between the beta branch and master. Not required by any means, but I thought it'd be nice.

Just reading this over, it looks to me that when running the test suite locally, a new version of stripe-mock is always started, even on the master branch (unless you have STRIPE_MOCK_PORT set in your environment, but I don't think most people do). Was that intentional? It seems like we should try to have it favor the already-running stripe-mock where possible for speed if nothing else.

We've often run into issues where some tests would fail after upgrading your local instance of stripe-mock (e.g. after running brew update && brew upgrade if you installed via brew), so I kind of like the idea of the test suite always running its own stripe-mock instance with the correct spec and fixtures files.

If we don't do this though, how would you recommend checking for master vs. beta branch? We could either just have different code in the beta branches, or do something like git rev-parse --symbolic-full-name --abbrev-ref HEAD to check if the current branch is master (but then you'd have to run the test suite from the git repo).

[brandur-stripe]

Yeah exactly. This way you'd have all the information in one place when looking at the diff between the beta branch and master. Not required by any means, but I thought it'd be nice.

Yep, makes sense.

We've often run into issues where some tests would fail after upgrading your local instance of stripe-mock (e.g. after running brew update && brew upgrade if you installed via brew), so I kind of like the idea of the test suite always running its own stripe-mock instance with the correct spec and fixtures files.

Those failures are generally well-documented at least right? As in, the test suite fails with "the minimum stripe-mock needed is X.X.X". (I'm not sure if there's some other type of "upgrade failure" that I can't think of.)

I sort of like the idea of continuing to fall back to the system's stripe-mock as much as possible just to keep things really, really fast. The test suites for languages like Ruby and Go are fast enough now that launching a new process every time would be a noticeable slow down. There also may be that problem of Mac OS popping up the "stripe-mock allow like to listen on port XXX, allow/deny" dialog all the time which would be pretty annoying.

If we don't do this though, how would you recommend checking for master vs. beta branch? We could either just have different code in the beta branches, or do something like git rev-parse --symbolic-full-name --abbrev-ref HEAD to check if the current branch is master (but then you'd have to run the test suite from the git repo).

Hm, a Git-based solution seems relatively plausible — I'd probably suggest a chance to your approach a bit by checking whether the current branch has any modifications in the directory containing the OpenAPI specs. That way non-beta branches on both your local machine and in CI could continue to use system stripe-mock.

I also think that it might not be that bad to make the launch explicit on beta branches. As in we add a line that's normally commented out like:

# Uncomment for an alpha/beta branch that need stripe-mock to use custom OpenAPI.
# Stripe::StripeMock.start

Admittedly though, there'd always be risk of it being accidentally merged to master.

[ob-stripe]

Okay, I've updated the PR with your feedback. Now the test suite looks for a spec3.json file in test/openapi/. If it exists, then the suite will start its own stripe-mock process with the custom spec file, otherwise it assumes that stripe-mock is already running.

This way, we can merge this PR into master and keep the existing behavior. In beta branches, all we need to do is include the custom spec3.json and fixtures3.json files in test/openapi/ and the test suite will start stripe-mock with those custom files instead of using the already running stripe-mock process.

ptal @brandur-stripe

[brandur-stripe]

Nice! LGTM.

I'm really liking not having the spec bundled in master and falling back on the system's stripe-mock by default. Hopefully I didn't end up pushing us in a direction that will overall be harder to use, but if I did, feel free to push back by iterating on the design in the future once we know more from having started to use it.

[ob-stripe]

Alright, thanks @brandur-stripe! Pulling this in.