Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Adds before/after hooks to Dredd #44

Merged
merged 12 commits into from
Mar 7, 2014
Merged

Conversation

ecordell
Copy link
Contributor

This pull request adds before/after test hooks to dredd. Everything is tested, and I did some minor refactoring to try and separate concerns a bit more.

I'll copy the documentation below for explanation:

Writing Hooks

Dredd can be configured to use a set of hook files (specified with the --hookfiles flag) that define before and after hooks for requests. Hookfiles can be in javascript or coffeescript, and must import the hook methods.

Requests are identified by their name, which is derived from the structure of the blueprint. You can print a list of the generated names with --names.

Example

Get Names:

$ dredd single_get.md http://machines.apiary.io --names
info: Machines > Machines collection > Get Machines

Write a hookfile:

{before, after} = require 'hooks'

before "Machines > Machines collection > Get Machines", (transaction) ->
  console.log "before"

after "Machines > Machines collection > Get Machines", (transaction) ->
  console.log "after"

Run tests:

$ dredd single_get.md http://machines.apiary.io --hookfiles=*_hooks.*
info: Found Hookfiles: test_hooks.coffee
info: Beginning Dredd testing...
before
pass: GET /machines duration: 2965ms
after
complete: 1 passing, 0 failing, 0 errors, 0 skipped
complete: Tests took 2977ms

@zdne
Copy link
Contributor

zdne commented Feb 21, 2014

@ecordell would you be open for a quick Skype / Hangout on this PR and overall Dredd concept? I would like to discuss with you the gist of Dredd and its possible future directions also in regards to this PR.

I feel this PR is continuing the trend that goes against the original idea of Dredd testing API Blueprint and I would love to discuss it with you as you are VIP in this project.

Thanks.

@zdne
Copy link
Contributor

zdne commented Mar 3, 2014

@ecordell

Sorry for the delay. I have finally found time to review this PR. I do not have any technical comments, so from this point it looks all good.

As per our Skype-discussion: what I would do is to move the "Writing Hooks" sections from the README.md to a Wiki article (and reference it from the README) to put a little bit less emphasis on hooks. Is this OK with you Evan?

No action necessary on your side, I will merge it, modify the README and create the Wiki entry for this.

After that I would publish a new Dredd release.

@zdne
Copy link
Contributor

zdne commented Mar 3, 2014

Note on Dredd design

(my thoughts on introducing hooks to Dredd)

The goal of Dredd is to test an API blueprint against the service it describes. Not to test an API per se. The difference may be subtle at a first glance but the implications and consequences are substantial. Consider you find a bug that occurs in your API implementation under some special circumstances. Should Dredd test it? Most likely not. If the special circumstances are so important you need to inform your API user about it then yes. You write it in your blueprint and Dredd tests it.

The rule of thumb is "Unless it is something user should know about it should not be tested by Dredd".

Ideally the Dredd should take only an API blueprint as its input and nothing else. No external knowledge, no hook files no cucumber features, nothing. Just a blueprint. The result should be yes or no – does the blueprint conform to the API it was run against? The hooks are out of band information and as such should be avoided.

With that being said. As of now the blueprint is not 100% ready for testing. With the implementation of apiaryio/api-blueprint#21 pending there is a gap in this story that hooks (this PR) can temporarily help to bridge.

Once API Blueprint supports scenarios and backgrounds the need for hooks should diminish. Later with scenarios & backgrounds in place hooks can still remain part of the Dredd to provide extension for future tools built upon it. But again the spirit and purity of Dredd in API testing driven by the API Blueprint should be preserved.

http://blog.apiary.io/2013/10/10/No-more-outdated-API-documentation
http://blog.apiary.io/2013/10/17/How-to-test-api-with-api-blueprint-and-dredd

@ecordell
Copy link
Contributor Author

ecordell commented Mar 3, 2014

@zdne Absolutely, sorry about that. I got a little overzealous with the --amend flag.

(also, I bumped the version to 0.3.0 because this adds features rather than fixes bugs, but if you want to de-emphasize the hooks it could be changed back to a 0.2.x)

I've recently identified a bug I introduced with this refactoring. This PR breaks dredd's cleanup if interrupted with a SIGINT (ctrl+c in the terminal). I'll write a test for it and a fix and push it up (I can make the other changes you mentioned at the same time, unless you just want to do it yourself).

@zdne
Copy link
Contributor

zdne commented Mar 3, 2014

No worries @ecordell , also I think it is OK to go with 0.3.0 as this is a new feature that still deserves version bump.

Please proceed as you want (including the Wiki entry if you will) and let me know when done. I will be happy to merge this!

Thanks

@ecordell
Copy link
Contributor Author

ecordell commented Mar 3, 2014

Moved the docs to the wiki and fixed the sigint bug. (I didn't write a good cli test for it because I couldn't figure out how to make a child process emit a SIGINT event - must be missing something.)

Also changed a couple of things so that multiple hooks can be registered for the same endpoint (pretty small change, and a test is included).

@zdne
Copy link
Contributor

zdne commented Mar 3, 2014

@ecordell

Awesome. Going to hit the sac. Will review and merge it tomorrow.

@zdne
Copy link
Contributor

zdne commented Mar 5, 2014

@ecordell

I was about to merge the PR. However during the review process there were some changes to master and this cannot be now merged.

I have tried merging it manually both using merge and rebase. Unfortunately seems your refractor of execute-transaction.coffee to transaction-runner.coffee seemingly was not done using git mv and thus I cannot project the fixes we have made to execute-transaction.coffee into transaction-runner.coffe.

To pull our latest master in your fork run:

$ git remote add upstream https://github.com/apiaryio/dredd.git
$ git pull upstream master

Now ideally we would need to rebase test-hooks over the master. But since the refactor it has to be done manually. Can you please take a look into it? Thank you and sorry about it.

Also refer to #49 and #45

@ecordell
Copy link
Contributor Author

ecordell commented Mar 5, 2014

Ah, I'll pull in the new changes and push up a merged version. I actually didn't know about git mv, good to know for the future. I try to do a tiny bit of refactoring each time I touch Dredd, so hopefully it's becoming more and more loosely coupled and 'DRY'.

Aside: Have you considered writing a sort of "Dredd manifesto" detailing the intended use of Dredd as a blueprint testing tool rather than writing detailed integration tests? (Could probably just put the text of your previous comment in the readme.)

@zdne
Copy link
Contributor

zdne commented Mar 5, 2014

@ecordell no worries, I will refrain from any commits to master until this PR is merged.

As for manifesto – Good point but I hope the README should say it all if not we (I) should modify it so it is more clear. Also the aforementioned blogposts by @netmilk should help here..

Will revisit the README when time allows. Thanks!

Evan Cordell added 3 commits March 5, 2014 17:51
Conflicts:
	src/execute-transaction.coffee
	test/integration/cli-test.coffee
	test/unit/execute-transaction-test.coffee
@ecordell
Copy link
Contributor Author

ecordell commented Mar 6, 2014

It should merge cleanly now. Also made a small addition to expose the transactions to a hookfile.

zdne added a commit that referenced this pull request Mar 7, 2014
Adds before/after hooks to Dredd
@zdne zdne merged commit 0939767 into apiaryio:master Mar 7, 2014
@BRMatt
Copy link

BRMatt commented Mar 7, 2014

Would it be possible, in the future, to separate "refactor" commits from the "new feature" commits for easier PR review (what is new code and

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants