-
Notifications
You must be signed in to change notification settings - Fork 151
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
Test: add recording #123
Test: add recording #123
Conversation
package.json
Outdated
@@ -112,7 +113,8 @@ | |||
"tslint": "^5.7.0", | |||
"tslint-config-standard": "^6.0.1", | |||
"typescript": "^2.5.3", | |||
"uuid": "^3.2.1" | |||
"uuid": "^3.2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure but didn't you remove all the uuid usage?
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
=======================================
Coverage 79.23% 79.23%
=======================================
Files 75 75
Lines 1999 1999
Branches 230 230
=======================================
Hits 1584 1584
Misses 323 323
Partials 92 92 Continue to review full report at Codecov.
|
@axe312ger Can you 👀 |
test/record.js
Outdated
module.exports = function (name, options) { | ||
// options tell us where to store our fixtures | ||
options = options || {}; | ||
const fixtureFolder = options.fixtureFoldr || 'fixtures'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in options config fixtureFoldr
// `hasFixture` indicates whether the test has fixtures we should read, | ||
// or doesn't, so we should record and save them. | ||
// the environment variable `NOCK_RECORD` can be used to force a new recording. | ||
let hasFixture = process.env.NOCK_RECORD === 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on your comment I'd expect it to detect if a fixture is present or not. But you just hardcoded it based on the environment variable. When I look at the npm scripts, it is always 0, so hasFixture would always be false, creating new fixtures every run. How does this work? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we want to refresh the fixtures if for example, we add a new endpoint so we change NOCK_RECORD
to 1
}, | ||
// saves our recording if fixtures didn't already exist | ||
after: function (done) { | ||
if (!hasFixture) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt you check this based if you have a recording available or not? Plus only record when you need to? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
before(recorder.before); | ||
after(recorder.after); | ||
|
||
const ENVIRONMENT_ID = 'env-integration'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on how easy it is to integrate <3
😻 |
This PR adds recording to the integration and en2en tests so users can run them on their machine without the need to hit the API endpoint