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

Formalizing the test-machine command API #230

Merged
merged 8 commits into from
Feb 13, 2020
Merged

Formalizing the test-machine command API #230

merged 8 commits into from
Feb 13, 2020

Conversation

cddr
Copy link
Contributor

@cddr cddr commented Feb 12, 2020

Rationale

The test-machine commands are "just data". This allows them to be created using whatever method is convenient (e.g. generated from the contents of some edn file, extracted from a database, read from an existing kafka topic etc.). However when authoring commands from scratch, it can be annoying to have to remember the syntax of each command (or keep referring back to the docs).

This PR adds some functions in the jackdaw.test.commands ns to provide a bit of help for this use-case. Having (spec'd) functions means that tools like cider can give us a bit of interactive help without the need to leave our editor. The specs are very basic for now but we can further refine them (particularly the ::write-options and ::watch-options specs) to provide even better assistance. As an example, the screenshot below is the result from M-x cider-doc on the jackdaw.test.commands/watch function.

Screen Shot 2020-02-12 at 5 01 17 PM

Checklist

  • tests (updated existing tests)
  • updated CHANGELOG (the "unreleased" section)

@cddr cddr requested a review from a team as a code owner February 12, 2020 15:28
@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

Merging #230 into master will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   80.20%   80.30%   +0.09%     
==========================================
  Files          42       42              
  Lines        2627     2660      +33     
  Branches      151      153       +2     
==========================================
+ Hits         2107     2136      +29     
- Misses        369      371       +2     
- Partials      151      153       +2     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d11762b...8cbc24d. Read the comment docs.

@99-not-out
Copy link
Contributor

Nice - good start to add hooks for further stuff, and the spec is a very helpful addition.

99-not-out
99-not-out previously approved these changes Feb 12, 2020
@AndreaCrotti
Copy link
Contributor

Nice work!
One question, if I read correctly all the spec validation will only be done if you use the new wrappers?
I think that maybe that adding a s/assert where the command gets executed (jd.test/run-test for example) could also be good, since it would check that instructions are well formed also when they are not generated using the wrappers.

@cddr
Copy link
Contributor Author

cddr commented Feb 13, 2020

I think that maybe that adding a s/assert where the command gets executed (jd.test/run-test for example) could also be good, since it would check that instructions are well formed also when they are not generated using the wrappers.

Yeah I think it's worth following up with that. The :ret specs here are pretty minimal (i.e. they just specify vector?) so if we make them a bit more stringent by specifying the structure of the returned vector, then we can re-use those specs in run-test.

@AndreaCrotti AndreaCrotti self-requested a review February 13, 2020 13:04
Copy link
Contributor

@AndreaCrotti AndreaCrotti left a comment

Choose a reason for hiding this comment

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

LGTM

@cddr cddr merged commit 8ae7dd6 into master Feb 13, 2020
@cddr cddr mentioned this pull request Feb 13, 2020
2 tasks
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.

3 participants