Improving CLI/SDK Testing

Joshua-Anderson commented 8 years ago

Current CLI/SDK Feature Workflow:

Open SDK PR
Create CLI PR pointing the SDK to fork of SDK PR
Merge SDK PR
Update CLI PR to point to master again
Problems:
No e2e testing on CLI PR. When the CLI PR is merged e2e will fail, stopping all merges until it gets fixed.
Not enough confidence to merge SDK PRs cause there isn’t enough testing on them. Updating SDK versions in the CLI PR is tedious and causes many test reruns, delaying things.
Proposal 1: Test SDK with CLI

In this proposal, the sdk runs e2e by building the CLI with the version of the SDK in the PR.

Workflow:

Open SDK PR
CI builds the CLI with this PR and runs e2e.
Open CLI PR
CI automatically points SDK version to the CLI PR (using ‘Requires’ syntax) and runs e2e. If the change requires an e2e PR, the e2e PR can point to the CLI PR and vice versa.
Merge SDK PR
Update SDK version in CLI PR
Merge CLI PR
Problems:
For the SDK e2e tests, new features are not tested because they do not exist in the client, so you still have to create a CLI PR at the same time to test the new features.
Complicated… it’s very unlikely an outside contributor could get this right.
Advantages:
Confidence that the CLI will not break the e2e tests.
Proposal 2: E2E tests use SDK, CLI uses a mock

In the proposal, the e2e tests use the SDK instead of the cli. The CLI has fairly little logic and can be tested fairly completely against a mock http server (like we currently do for the SDK). This would allow us to check the CLI maintains session, parses command line arguments correctly, and etc.

Workflow:

Open SDK PR and a corresponding e2e PR using ‘Requires’ syntax.
Merge SDK PR
Open CLI PR
Merge CLI PR
Problems:
Lots of work to rewrite e2e
Not easy to work on CLI and SDK simultaneously (runs into many of the problems of our current workflow). A feature requires both an SDK and CLI update, and it makes sense to write both at the same time.
Advantages:
Confidence that the CLI/SDK will not break the e2e tests.
Pretty simple to understand, at least in relation to the rest of deis projects.

Proposal 3: E2E tests for both SDK and CLI

Write e2e tests for the SDK, but keep the CLI tests for normal e2e.

Workflow:
Open SDK PR and a corresponding e2e PR using ‘Requires’ syntax.
Merge SDK PR
Open CLI PR and a corresponding e2e PR using ‘Requires’ syntax.
Merge CLI PR
Problems:
Lots of work to write SDK e2e and keep both up to date
Not easy to work on CLI and SDK simultaneously (runs into many of the problems of our current workflow). A feature requires both an SDK and CLI update, and it makes sense to write both at the same time.
Advantages:
Confidence that the CLI/SDK will not break the e2e tests.
Pretty simple to understand, at least in relation to the rest of deis projects.

vdice commented 8 years ago

I am in favor of Proposal 1 at this time, with the initial pain points of this approach hopefully inspiring tooling improvements to make the experience better.

arschles commented 8 years ago

Feedback from offline discussion:

We'd like to pursue item (1)
We'd like to have a new jenkins job that watches for PRs on all of the following repos:
When this job fires for a given PR, it looks for the associated PRs from the other 2 repos
- PRs are associated if they have the Requires syntax in them, and those pointers create a DAG
- For example, if a CLI PR is submitted, the associated e2e PR is the one that points to it and the associated SDK PR is the one that the CLI PR points to
The test status on all PRs will be red until all 3 PRs are up, they create a DAG that starts with the E2E tests

mboersma commented 8 years ago

Just to chime in, I'm also in favor of less disruptive, less effort (1), especially since my assumption is that this type of multi-PR situation will become somewhat less common in the future.

bacongobbler commented 8 years ago

If we're going the route of option 1, we might as well not do anything. It's just as complicated to get the CLI to build with a specific SDK version than it is to manually pin to a version in glide.yaml like we already do. Essentially we'd be doing the same thing, except we're engineering this into CI and wasting release cycles. I don't think it's worth the time or effort to go that route if a simple change in glide.yaml is all it takes to effectively do all this work for us.

bacongobbler commented 8 years ago

As for this:

CI automatically points SDK version to the CLI PR (using ‘Requires’ syntax) and runs e2e. If the change requires an e2e PR, the e2e PR can point to the CLI PR and vice versa. Merge SDK PR Update SDK version in CLI PR Merge CLI PR

This already exists, so the pain points are from

Open SDK PR CI builds the CLI with this PR and runs e2e. Open CLI PR

Which is accomplished by modifying glide.yaml.

Joshua-Anderson commented 8 years ago

@bacongobbler The main thing we are working on is get one e2e running on CLI and SDK repos, and also possibly a single e2e build for the corresponding cli/sdk/e2e PR using the requires syntax, making it less painful to have to make the 3 part merges.

Joshua-Anderson commented 8 years ago

That being said, I think standalone controller SDK integration tests could be valuable, because right now as a SDK developer it is really hard to test that your changes actually work against a real cluster.

bacongobbler commented 8 years ago

As for the alternate options: I'm completely against options 2 and 3 for a couple reasons:

1) mocking out the CLI for option 2 essentially throws away our existing (and green) e2e. Not a good idea. 2) e2e for both the cli and the sdk means contributors must now come up with two implementations in the same codebase for the same problem. As if it already isn't difficult enough to get people to contribute e2e tests...

I would propose a 4th option: just write integration tests for the sdk and leave e2e alone. That way SDK devs can test their changes against their own cluster, but we don't necessarily need to validate them against any sort of 2e2 for now. Just more of "helper" tests for devs.

vdice commented 8 years ago

Initial pipeline now created in https://github.com/deis/controller-sdk-go/issues/39; closing.

deis / workflow