Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize step inputs and outputs #19

Merged
merged 21 commits into from
Jun 26, 2018
Merged

Conversation

jstokes
Copy link
Member

@jstokes jstokes commented Jun 24, 2018

Add the ability to specify test function inputs and outputs declaratively. ::step/inputs can be either components from the system or resolved from the context. Lookups in the context can be by keyword, a collection of values or an arbitrary function. Outputs can similarly be under a keyword, nested keys for assoc-in, or a function of the previous context and test result. For #17 and #18.

Rev bump as this is breaking backwards compatibility.

It seems possible to have a static check for any missing system components before any steps have run, but not context inputs.

Copy link
Contributor

@aengelberg aengelberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I was thinking we could just throw the inputs into the global step map, taking advantage of namespaced keys to differentiate the keyspaces:

#::step
{:name 'my-step
 :tenant/id (step/lookup :tenant/id)
 :tenant/client (step/component :tenant/client)
 ...}

That said, I can't think of anything wrong with your strategy (having a ::step/inputs map for lookups and components) since it's still easy to set defaults in step constructors and override them from test suites. ¯_(ツ)_/¯

project.clj Outdated
@@ -1,4 +1,4 @@
(defproject amperity/greenlight "0.1.0-SNAPSHOT"
(defproject amperity/greenlight "0.2.0-SNAPSHOT"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it makes sense to bump the version number when we haven't even cut an official 0.1.0 yet.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's good for libraries to bump versions when they introduce backwards incompatible code. this lets test authors upgrade when they're ready (instead of their tests just breaking until they update their code)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nvm. i misunderstood what you were advocating.

Copy link
Contributor

@aengelberg aengelberg Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I agree, but since we're still in snapshot-land I think breaking changes are fair game. And since this repo is still private, no one besides us is using the library anyway. The question is how to cause minimal pain to our internal code. My proposal is:

  1. In our repo, change all instances of [amperity/greenlight "0.1.0-SNAPSHOT"] to [amperity/greenlight "0.1.0-20180601.162327-5"] to lock in the specific version id we care about, thus protecting against future breaking snapshot releases
  2. Don't touch the version at all in this pull request, just release the changes as another 0.1.0-SNAPSHOT
  3. Gradually upgrade our integration test projects' dependencies on Greenlight from "0.1.0-20180601.162327-5" back to "0.1.0-SNAPSHOT" (or maybe the new specific version id) as we update our usage of Greenlight accordingly
  4. If we feel good about the library revamp after a week or two, bump to "0.1.0" and share with the world

:kw (assoc ctx output-key %)
:kws (assoc-in ctx output-key %)
:fn (output-key ctx %)))
ctx)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems potentially worth breaking out into a helper function to match collect-inputs.

;; a collection of values as keys, or a function (ctx, return-value) -> ctx'
(s/def ::output
(s/or :kw keyword?
:kws (s/coll-of any? :min-count 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you really want a sequential collection (not a set or a map), maybe (s/and sequential? seq) would be better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or (s/coll-of any? :min-count 1 :kind sequential?)

@seako
Copy link

seako commented Jun 25, 2018

are we keeping a changes.md? that seems like a good place to document the change that took place in 0.1.0 -> 0.2.0 and how to update your code to the new way of doing things.

@jstokes
Copy link
Member Author

jstokes commented Jun 25, 2018

are we keeping a changes.md?

For sure, need to create a CHANGELOG.md. Will introduce with the 0.1.0 release, predating these changes.

(s/conform ::inputs (::inputs step {}))))


(defn- collect-outputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this name makes sense since there's only one output and you aren't really "collecting" it. Maybe save-output or register-output?

Copy link
Contributor

@greglook greglook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM. Could use some polish like docstrings, updating the README and CHANGELOG files, but the structure of the code looks good to me. The next thing we should consider is standardizing the step-constructor patterns - it seems like being able to override the inputs/output values is going to be the most common usage of extra constructor args.

exception if not all components are available."
[system step]

(defn- resolve-context!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions could use some docstrings.

@@ -0,0 +1,54 @@
(ns greenlight.test-test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yo dawg, I heard you liked tests...

@jstokes jstokes merged commit bad3061 into master Jun 26, 2018
@jstokes jstokes deleted the generalize-inputs-outputs branch June 26, 2018 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants