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

Add status codes to test ID and Dredd output #868

Merged
merged 3 commits into from
Aug 23, 2017

Conversation

anth0d
Copy link

@anth0d anth0d commented Aug 18, 2017

πŸš€ Why this change?

The primary reason for this change is that test IDs are non-unique and that makes test output hard to understand. This PR adds the (###) to the transaction ID so that a 200 is distinctly different from a 204, 401, 404, etc...

info: Successfully connected to hooks handler. Waiting 0.1s to start testing.
pass: POST (204) /channelmapper/v1/channel duration: 25ms
skip: POST (401) /channelmapper/v1/channel
skip: POST (404) /channelmapper/v1/channel
pass: GET (200) /v1/channel/9cd4342be duration: 25ms
skip: GET (401) /v1/channel/9cd4342be
pass: GET (404) /v1/channel/9cd4342be duration: 16ms

complete: 3 passing, 0 failing, 0 errors, 3 skipped, 6 total

πŸ“ Related issues and Pull Requests

βœ… What didn't I forget?

  • [βœ”] To write docs
  • [βœ”] To write tests
  • [βœ”] To put Conventional Changelog prefixes in front of all my commits and run npm run lint

@anth0d anth0d changed the title Origin/other status codes Add status codes to test ID and Dredd output Aug 18, 2017
Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think this is generally a good idea. See my comments.

The current format of the transaction ID is part of many tests, so those will need to be changed first. That's why tests on both Windows (AppVeyor) and Linux (Travis CI) failed.

I also suggest to update the example value for the transaction ID in Dredd's docs: https://dredd.readthedocs.io/en/latest/data-structures.html#transaction-object

package.json Outdated
@@ -12,6 +12,7 @@
"scripts": {
"lint": "conventional-changelog-lint --from=master && coffeelint src",
"build": "coffee -b -c -o lib/ src/",
"watch": "coffee -w -b -c -o lib/ src/",
Copy link
Contributor

Choose a reason for hiding this comment

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

I was reading #867 before this one, hence my comment there. The only important part is that I'd suggest the name of the command to be build:watch instead of watch.

package.json Outdated
@@ -35,7 +36,7 @@
"coffee-script": "^1.12.5",
"colors": "^1.1.2",
"cross-spawn": "^5.0.1",
"dredd-transactions": "^4.3.0",
"dredd-transactions": "4.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I thought I linked the issue. Currently the latest version of dredd-transactions has an issue introduced by one of its dependencies - apiaryio/dredd-transactions#93

@honzajavorek
Copy link
Contributor

honzajavorek commented Aug 18, 2017 via email

@anth0d
Copy link
Author

anth0d commented Aug 18, 2017

@honzajavorek I've made the changes you requested - but unfortunately the Travis CI build failed - I am trying to reproduce but having no success. The failing tests are here and here if you have any tips for how to debug.

@anth0d anth0d closed this Aug 18, 2017
@anth0d anth0d reopened this Aug 18, 2017
@honzajavorek
Copy link
Contributor

@antkazam I see all builds passing on the last commit. I guess you figured it out then? I'll get back shortly to do review.

@honzajavorek
Copy link
Contributor

Just a nitpick from looking at the commits, you may want to fix your git or GitHub email settings to have your authorship correctly associated:

image

Only noting for your own good, this by no means blocks the PR.

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

Looks great! πŸ‘ Thanks so much! ✨

I'm ready to merge this. I'll give you some time to figure out the email associations if you want (if you want to amend the previous commits and to change their email, you may want to rebase the branch before it gets merged) and possibly tomorrow I think I'll merge this πŸš€

@honzajavorek
Copy link
Contributor

This needs to wait until #872 gets fixed. We need to release a fix in patch so it auto-propagates to installations the same way as did the bug. Adding a feature would close the way to auto-propagate by bumping the minor version number.

@anth0d
Copy link
Author

anth0d commented Aug 23, 2017

Thanks @honzajavorek I was out of town until now. Really appreciate your help.

@anth0d anth0d deleted the origin/other-status-codes branch August 23, 2017 15:40
@honzajavorek
Copy link
Contributor

@antkazam Thanks for the PR! πŸ‘

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.

Transaction ids are not unique and do not include expected status code
2 participants