chore: drop sinon as devDep: all mocks are now through nock. #173
Conversation
Also, remove dead test function for getting an access token.
WalkthroughThe PR shifts tests from callback/Sinon/real-API usage to promise/async style with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test (Mocha)
participant C as Strava Client
participant N as Nock (HTTP Mock)
Note over T,N: Tests use promises/async and nock to intercept HTTP calls
T->>C: call client.activities.create/get/update(...)\n(return Promise)
C->>N: HTTP request (POST/GET/PUT) with Authorization header
N-->>C: Mocked HTTP response (200 / 401)
C-->>T: Promise resolves with payload or rejects with error
alt Unmatched or missing mock
C->>N: Unmatched HTTP request
N-->>T: Error (test fails)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)package.json(0 hunks)test/_helper.js(0 hunks)test/activities.js(3 hunks)
💤 Files with no reviewable changes (2)
- package.json
- test/_helper.js
🧰 Additional context used
🧬 Code graph analysis (1)
test/activities.js (3)
test/oauth.js (3)
nock(5-5)strava(4-4)payload(66-66)index.js (2)
require(18-18)strava(21-21)lib/oauth.js (1)
require(2-2)
- Added Wesley Schlenker to the list of contributors. - Updated README to reflect the use of `yarn test` instead of `npm test` for running the test suite.
So no live Strava API key is required.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/client.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/client.js (2)
test/oauth.js (4)
should(1-1)nock(5-5)strava(4-4)payload(66-66)index.js (2)
require(18-18)strava(21-21)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/activities.js (3)
8-11: Assert mocks are consumed and prevent accidental live network.
- Add nock.isDone() verification to catch unconsumed interceptors.
- Disable outbound network in this suite and restore afterward to guard against real calls.
Apply this diff:
before(function () { // Set a default test activity to use in tests testActivity = { id: 123456789, resource_state: 3, name: 'Sample Activity' } + // Prevent accidental live network in this suite (restored in `after`) + nock.disableNetConnect() }) - afterEach(function () { - // Clean all nock interceptors after each test - nock.cleanAll() - }) + afterEach(function () { + // Assert all expected HTTP mocks were used + const done = nock.isDone() + const pending = done ? [] : nock.pendingMocks() + nock.cleanAll() + if (!done) { + throw new Error('Not all expected HTTP mocks were called: ' + pending.join(', ')) + } + }) + + after(function () { + // Restore network behavior for other suites + nock.enableNetConnect() + })Also applies to: 13-16
50-66: Avoid inter-test coupling on testActivity.id.These tests depend on the prior create() test mutating testActivity. Prefer per-test known IDs or reset state in a beforeEach to make tests independent.
68-83: Duplicate of the previous get() test.This repeats the same Promise flow and assertions. Consider removing to keep the suite lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/activities.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/activities.js (2)
test/client.js (4)
nock(3-3)require(4-4)strava(5-5)should(2-2)test/oauth.js (4)
nock(5-5)strava(4-4)payload(66-66)should(1-1)
🔇 Additional comments (5)
test/activities.js (5)
2-2: Adopting nock for HTTP mocking looks good.Import and usage are consistent with the new mocking approach.
85-100: Good verification of explicit token handling.Precise header match ensures access_token passthrough is tested.
111-120: Update flow mock and assertions look correct.Covers both resource_state and name.
Also applies to: 121-125
28-41: Nice end-to-end create() mock. Consider stabilizing token source.Right now, most tests match any Bearer token. To make tests fully self-contained, either:
- pass an explicit access_token in the call and match exactly in nock, or
- seed STRAVA_ACCESS_TOKEN only when unset and restore it after (suite-level).
If you prefer the env approach, verify whether any suite already sets STRAVA_ACCESS_TOKEN. You can run:
#!/bin/bash # Inspect tests for env token handling rg -n -C2 -g '!**/node_modules/**' -P 'process\.env\.STRAVA_ACCESS_TOKEN' testAlso applies to: 42-47
170-178: LGTM on laps/comments/kudos mocks.Clean, minimal responses with header checks; combined with the isDone() assertion, these will be reliable.
Also applies to: 179-183, 187-194, 195-199, 205-210, 211-215
|
I think Code Rabbit flagged a real bug here, which is that we have docs and a code comment that says we reject non-2xx responses with an error of type StatusCodeError, but it appears we don't actually do so... but it's not as simple a fix as just starting to do so, because we also have code about updating rate limits... which suggests in fact maybe there are some rate-limited HTTP response codes which we should maybe also treat as success instead of a status code error? Also, now we have >50% of the test suite running without a live Strava API key, which means we could set up Github Actions to have it at at least run the tests that pass with mocks, then we can continue to work on mock'ing the API until we can run 100% of the test suite on mocks. Mocks have their downsides, but the reality is that often now we don't run the tests at all due to the live API key requirement. I think with Nock there may be a way that once we are completely mocked, there may a way we can enable people to disable all the mocks globally and use a real API key if they want to. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/client.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/client.js (1)
test/oauth.js (4)
should(1-1)nock(5-5)strava(4-4)payload(66-66)
🪛 Biome (2.1.2)
test/client.js
[error] 76-76: Expected a catch clause but instead found ')'.
Expected a catch clause here.
(parse)
[error] 78-78: expected a name for the function in a function declaration, but found none
(parse)
🔇 Additional comments (5)
test/client.js (5)
7-9: LGTM!Explicit test tokens improve test isolation and eliminate file system dependencies.
12-15: LGTM!The
afterEachhook withnock.cleanAll()ensures proper cleanup of HTTP mocks between tests, preventing test interference.
20-35: LGTM!The test correctly verifies that non-2xx responses are rejected with
StatusCodeError. Theshould.fail()guard on Line 30 ensures the test cannot false-pass if the rejection doesn't occur.
37-56: LGTM!The test correctly mocks a successful athlete data retrieval and validates the response payload structure.
78-99: LGTM once the syntax error is fixed.This test correctly validates successful athlete data retrieval with a different mock response. It will function properly once the duplicate
trystatement on Line 67 is removed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/activities.js (2)
68-83: Remove redundant promise-based test.Both
#get()tests now use the same promise-based pattern after the Sinon-to-nock migration. The "(Promise API)" label is misleading since both tests are promise-based. Consider removing this duplicate test to reduce maintenance overhead.
19-215: Consider verifying nock interceptors were used.While the tests correctly clean up nock with
cleanAll(), they don't verify that all mocked requests were actually made. Callingscope.done()after each test assertion helps catch scenarios where expected HTTP calls never occurred.Based on learnings
Example pattern for one test:
it('should create an activity', function () { var args = { name: 'Most Epic Ride EVER!!!', elapsed_time: 18373, distance: 1557840, start_date_local: '2013-10-23T10:02:13Z', type: 'Ride' } // Mock the create activity API call - nock('https://www.strava.com') + const scope = nock('https://www.strava.com') .post('/api/v3/activities') .matchHeader('authorization', /Bearer .+/) .reply(201, { id: 987654321, resource_state: 3, name: 'Most Epic Ride EVER!!!', elapsed_time: 18373, distance: 1557840, start_date_local: '2013-10-23T10:02:13Z', type: 'Ride' }) return strava.activities.create(args) .then(function (payload) { testActivity = payload; (payload.resource_state).should.be.exactly(3) + scope.done() }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/activities.js(3 hunks)
🔇 Additional comments (1)
test/activities.js (1)
8-16: LGTM! Proper test setup and cleanup.The before hook initializes a concrete test activity, and the afterEach hook correctly cleans up nock interceptors between tests.
|
@wesleyschlenker I get failures here for the athletes tests. Do they pass for you? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/client.js (5)
12-15: Consider strengthening mock isolation with network controls.The
afterEachcleanup is good, but tests could accidentally hit the real Strava API if nock mocks fail to match requests.Add
nock.disableNetConnect()in abeforeEachhook to ensure tests never make real network calls:describe('client_test', function () { + beforeEach(() => { + // Prevent any real network requests + nock.disableNetConnect() + }) + afterEach(() => { // Clean up after each test nock.cleanAll() })Based on learnings: "Use nock.disableNetConnect() to ensure tests never make real network calls" and "Keep mocks isolated per-test and call nock.cleanAll() in test teardown."
20-35: Verify mock consumption to catch test logic errors.The test correctly asserts error behavior, but doesn't verify the nock mock was actually consumed. If the mock fails to match, the test might hit unexpected code paths.
Capture the nock scope and verify it was consumed:
it('Should reject promise with StatusCodeError for non-2xx response', async function () { // Mock athlete endpoint for BAD token -> 401 - nock('https://www.strava.com') + const scope = nock('https://www.strava.com') .get('/api/v3/athlete') .matchHeader('authorization', 'Bearer ' + BAD_TOKEN) .reply(401, { message: 'Authorization Error', errors: [{ resource: 'Application', code: 'invalid' }] }) const badClient = new strava.client(BAD_TOKEN) try { await badClient.athlete.get({}) should.fail('Expected athlete.get to reject with StatusCodeError') } catch (err) { should(err).be.instanceOf(StatusCodeError) should(err.statusCode).equal(401) } + + // Verify the mock was consumed + scope.done() })Based on learnings: "Use scope.done() or nock.isDone() in teardown to assert expected requests were made."
37-56: Verify mock consumption for test correctness.Same issue: without verifying the mock was consumed, the test could pass even if the mock wasn't used as expected.
Apply this pattern:
it('should return detailed athlete information about athlete associated to access_token', function () { // Mock athlete endpoint for GOOD token -> 200 - nock('https://www.strava.com') + const scope = nock('https://www.strava.com') .get('/api/v3/athlete') .matchHeader('authorization', 'Bearer ' + GOOD_TOKEN) .reply(200, { resource_state: 3, id: 12345, firstname: 'Test', lastname: 'User' }) const client = new strava.client(GOOD_TOKEN) return client.athlete.get({}) .then((payload) => { should(payload).be.ok() should(payload.resource_state).equal(3) should(payload.id).equal(12345) + + // Verify the mock was consumed + scope.done() }) })
58-75: Verify mock consumption.it('Should reject promise with StatusCodeError when using bad token', async function () { // Mock athlete endpoint for BAD token -> 401 // Testing with a second interceptor to ensure nock works correctly - nock('https://www.strava.com') + const scope = nock('https://www.strava.com') .get('/api/v3/athlete') .matchHeader('authorization', 'Bearer ' + BAD_TOKEN) .reply(401, { message: 'Authorization Error' }) const badClient = new strava.client(BAD_TOKEN) try { await badClient.athlete.get({}) should.fail('Expected athlete.get to reject with StatusCodeError') } catch (err) { should(err).be.instanceOf(StatusCodeError) should(err.statusCode).equal(401) should(err.data.message).equal('Authorization Error') } + + scope.done() })
77-98: Verify mock consumption.it('Should successfully return athlete data with valid token', function () { // Mock athlete endpoint for GOOD token -> 200 // Testing a second successful request to verify client instances work correctly - nock('https://www.strava.com') + const scope = nock('https://www.strava.com') .get('/api/v3/athlete') .matchHeader('authorization', 'Bearer ' + GOOD_TOKEN) .reply(200, { resource_state: 3, id: 67890, firstname: 'Another', lastname: 'Athlete' }) const client = new strava.client(GOOD_TOKEN) return client.athlete.get({}) .then((payload) => { should(payload).be.ok() should(payload.resource_state).equal(3) should(payload.id).equal(67890) should(payload.firstname).equal('Another') + + scope.done() }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/_helper.js(1 hunks)test/client.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-26T14:09:38.947Z
Learnt from: markstos
PR: node-strava/node-strava-v3#173
File: README.md:390-392
Timestamp: 2025-09-26T14:09:38.947Z
Learning: The node-strava-v3 repository contains a script at `scripts/generate-access-token.js` that helps users generate access tokens for testing purposes.
Applied to files:
test/_helper.js
🧬 Code graph analysis (1)
test/client.js (1)
test/oauth.js (4)
should(1-1)nock(5-5)strava(4-4)payload(66-66)
🔇 Additional comments (3)
test/client.js (2)
2-3: LGTM! Proper imports for nock-based testing.The addition of
shouldandnockimports supports the migration to promise-based mocking.
7-9: LGTM! Explicit test tokens improve test clarity.Using explicit token constants instead of file-based loading makes tests more isolated and easier to understand.
test/_helper.js (1)
1-1: LGTM! Clean migration from file-based token loading; no remaining getAccessToken() references in test files.
|
The Code Rabbit "nitpick" feedback here looks like a good quality improvement. |
|
@markstos I get failures starting in the I hadn't realized you've assigned this PR to me, so I will try to carve out some time later. |
|
The tests should pass on this branch are the ones that are modified and mocked, which are test/client.js and test/actitivites.js, but I currently get test failures in the client test. Looks like we are now throwing errors a different way that the test suite is not expecting. |
2 participants
Also, remove dead test function for getting an access token.
Summary by CodeRabbit