Refactored cucumber steps into separate files #610
Conversation
WalkthroughThis change removes the large, monolithic Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (18)
features/step_definitions/like_steps.js (1)
27-42: Extract duplicated inbox fetching code into a helper functionThis inbox fetching logic is duplicated in the "should not be liked" step below. Consider extracting it to a reusable helper function to avoid duplication.
+ async function fetchInboxAndFindObject(testContext, objectName) { + const response = await fetchActivityPub( + 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', + { + headers: { + Accept: 'application/ld+json', + }, + }, + ); + const inbox = await response.json(); + const object = testContext.objects[objectName]; + const found = inbox.items.find((item) => item.object.id === object.id); + return { inbox, object, found }; + } Then('the object {string} should be liked', async function (name) { - const response = await fetchActivityPub( - 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', - { - headers: { - Accept: 'application/ld+json', - }, - }, - ); - const inbox = await response.json(); - const object = this.objects[name]; - - const found = inbox.items.find((item) => item.object.id === object.id); + const { found } = await fetchInboxAndFindObject(this, name); + assert(found, `Object with ID ${object.id} not found in inbox`); assert(found.object.liked === true); });features/support/activitypub.js (3)
3-51: Add function documentation and consider using iteration instead of recursionThis helper function implements a retry mechanism with exponential backoff, which is great for handling asynchronous operations. Consider adding JSDoc documentation and using iteration instead of recursion to avoid potential stack overflow issues with deep recursion.
/** + * Wait for an ActivityPub activity to appear in the inbox + * @param {Object} activity - The activity to wait for + * @param {Object|null} object - Optional object to further filter the activity + * @param {Object} options - Options for retry behavior + * @param {number} options.retryCount - Current retry count + * @param {number} options.delay - Current delay in ms + * @returns {Promise<void>} - Resolves when activity is found or throws error after max retries + */ export async function waitForInboxActivity( activity, object = null, options = { retryCount: 0, delay: 0, }, ) { const MAX_RETRIES = 5; + // Use iteration instead of recursion + let currentRetry = options.retryCount; + let currentDelay = options.delay; + + while (currentRetry <= MAX_RETRIES) { const response = await fetchActivityPub( 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', { headers: { Accept: 'application/ld+json', }, }, ); const inbox = await response.json(); if ( inbox.items.find((item) => { const activityFound = item.id === activity.id; if (object) { return activityFound && item.object.id === object.id; } return activityFound; }) ) { return; } - if (options.retryCount === MAX_RETRIES) { + if (currentRetry === MAX_RETRIES) { throw new Error( `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the inbox`, ); } - if (options.delay > 0) { - await new Promise((resolve) => setTimeout(resolve, options.delay)); + if (currentDelay > 0) { + await new Promise((resolve) => setTimeout(resolve, currentDelay)); } - await waitForInboxActivity(activity, object, { - retryCount: options.retryCount + 1, - delay: options.delay + 500, - }); + currentRetry++; + currentDelay += 500; + } }
13-21: Add error handling for HTTP responseConsider adding error handling to check the HTTP response status before processing the JSON.
const response = await fetchActivityPub( 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', { headers: { Accept: 'application/ld+json', }, }, ); + if (!response.ok) { + throw new Error(`Failed to fetch inbox: ${response.status} ${response.statusText}`); + } const inbox = await response.json();
11-11: Consider making MAX_RETRIES configurableThe MAX_RETRIES constant is hardcoded. Consider making it configurable through the options parameter for more flexibility.
- const MAX_RETRIES = 5; + const MAX_RETRIES = options.maxRetries || 5;features/step_definitions/account_steps.js (2)
5-21: Add error handling for HTTP response and group assertions by categoryConsider adding error handling for the HTTP response status and organizing assertions by category for better readability.
Then('the response contains {string} account details', async function (name) { + if (!this.response.ok) { + throw new Error(`Failed to get account details: ${this.response.status} ${this.response.statusText}`); + } const responseJson = await this.response.clone().json(); const actor = name === 'Our' ? this.actors.Us : this.actors[name]; + // String properties assert.equal(responseJson.name, actor.name); assert.equal(responseJson.handle, actor.handle); assert.equal(responseJson.bio, actor.summary); assert.equal(responseJson.url, actor.url); assert.equal(responseJson.avatarUrl, actor.icon?.url || ''); assert.equal(responseJson.bannerImageUrl, actor.image?.url || ''); + // Numeric properties assert.equal(typeof responseJson.postCount, 'number'); assert.equal(typeof responseJson.likedCount, 'number'); assert.equal(typeof responseJson.followingCount, 'number'); assert.equal(typeof responseJson.followerCount, 'number'); + // Boolean properties assert.equal(typeof responseJson.followedByMe, 'boolean'); assert.equal(typeof responseJson.followsMe, 'boolean'); });
23-33: Add type checking for data table valuesThe current implementation assumes all values from the data table are strings. Consider adding type checking or conversion based on expected field types.
Then('the response contains the account details:', async function (data) { + if (!this.response.ok) { + throw new Error(`Failed to get account details: ${this.response.status} ${this.response.statusText}`); + } const responseJson = await this.response.clone().json(); + const dataHash = data.rowsHash(); - for (const [key, value] of Object.entries(data.rowsHash())) { + for (const [key, rawValue] of Object.entries(dataHash)) { + let expectedValue = rawValue; + + // Convert string values to appropriate types + if (rawValue === 'true') expectedValue = true; + else if (rawValue === 'false') expectedValue = false; + else if (!isNaN(rawValue) && rawValue !== '') expectedValue = Number(rawValue); + assert.equal( responseJson[key], - value, - `Expected ${key} to be "${value}" but got "${responseJson[key]}"`, + expectedValue, + `Expected ${key} to be "${expectedValue}" but got "${responseJson[key]}"`, ); } });features/step_definitions/note_steps.js (2)
86-93: Add property existence check before assertion.The assertions directly access nested properties without first verifying they exist, which could lead to confusing errors if the response structure is unexpected.
Then( 'note {string} has the image URL {string}', function (activityName, expectedImageUrl) { const activity = this.activities[activityName]; + assert(activity && activity.object && activity.object.attachment, 'Activity object should have an attachment'); assert.equal(activity.object.attachment.url, expectedImageUrl); assert.equal(activity.object.attachment.type, 'Image'); }, );
35-84: DRY opportunity: Similar code pattern in note creation steps.Similar to the reply steps, there's duplicated code between the two note creation methods (with and without image).
Consider extracting common logic to a helper function:
import assert from 'node:assert'; import { Then, When } from '@cucumber/cucumber'; import { fetchActivityPub } from '../support/request.js'; +async function createNote(context, noteName, noteContent, imageUrl = null) { + const body = { content: noteContent }; + + if (imageUrl) { + body.imageUrl = imageUrl; + } + + context.response = await fetchActivityPub( + 'http://fake-ghost-activitypub.test/.ghost/activitypub/actions/note', + { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + }, + body: JSON.stringify(body), + }, + ); + + if (context.response.ok) { + const activity = await context.response.clone().json(); + context.activities[noteName] = activity; + context.objects[noteName] = activity.object; + } +} // ... existing code ... When( 'we create a note {string} with the content', async function (noteName, noteContent) { + await createNote(this, noteName, noteContent); - this.response = await fetchActivityPub( - 'http://fake-ghost-activitypub.test/.ghost/activitypub/actions/note', - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - content: noteContent, - }), - }, - ); - - if (this.response.ok) { - const activity = await this.response.clone().json(); - - this.activities[noteName] = activity; - this.objects[noteName] = activity.object; - } }, ); When( 'we create a note {string} with imageUrl {string} and content', async function (noteName, imageUrl, noteContent) { + await createNote(this, noteName, noteContent, imageUrl); - this.response = await fetchActivityPub( - 'http://fake-ghost-activitypub.test/.ghost/activitypub/actions/note', - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - content: noteContent, - imageUrl: imageUrl, - }), - }, - ); - - if (this.response.ok) { - const activity = await this.response.clone().json(); - - this.activities[noteName] = activity; - this.objects[noteName] = activity.object; - } }, );features/step_definitions/request_steps.js (2)
68-87: Consider adding error handling for file operations.The file reading operation could potentially fail, but there's no specific error handling. Adding a try/catch block would make debugging easier if there are issues with the test fixture file.
When( /^an authenticated (\"(post|put)\"\s)?request is made to "(.*)" with an image$/, async function (method, path) { + try { const image = await readFile(resolve(__dirname, '../fixtures/dog.jpg')); const formData = new FormData(); formData.append( 'file', new File([image], 'dog.jpg', { type: 'image/jpeg' }), ); this.response = await fetchActivityPub( `http://fake-ghost-activitypub.test${path}`, { method: method || 'post', body: formData, }, ); + } catch (error) { + console.error('Error during image upload test:', error); + throw error; + } }, );
50-140: Well-structured HTTP request steps with good error messaging.The remaining steps are implemented well with:
- Clear separation of request types (authenticated, with data, with image, unauthenticated)
- Detailed error messages in assertions
- Proper handling of multipart form data for image uploads
- Explicit response status code checks
As an additional improvement, consider extracting the common base URL as a constant:
import { fetchActivityPub } from '../support/request.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); +const BASE_URL = 'http://fake-ghost-activitypub.test'; // Then use ${BASE_URL} in place of 'http://fake-ghost-activitypub.test' throughout the filefeatures/step_definitions/webhook_steps.js (2)
80-90: Inconsistent media-type with the other two requestsThe unsigned request uses
application/ld+json, while the signed variants useapplication/json. Unless the endpoint actively distinguishes between the two, stick to a single content-type to avoid false negatives.
22-32: Nested-property helper does not handle array indices
authors.0.nameor similar paths will be treated as a literal object key"0"instead of an array index, leading to shape mismatches. Consider a small enhancement that detects numeric segments and builds arrays when needed.features/step_definitions/post_steps.js (1)
78-88: Only top-level keys are supported
post {n} has {key} set to {value}fails for nested paths such asauthor.name.
You can reuse the dotted-path reducer pattern already present in other step files:- assert( - String(post[key]) === String(value), + const actual = key.split('.').reduce((obj, k) => obj?.[k], post); + assert( + String(actual) === String(value), `Expected post ${postNumber} to have ${key} ${value}`, );features/step_definitions/activitypub_steps.js (1)
246-266: Recursive polling risks stack growth – switch to a loop
waitForOutboxActivityperforms up to five self-calls.
While the depth is small, replacing recursion with an iterative loop avoids needless stack frames and future “max call stack” surprises if the retry limit ever increases.features/step_definitions/follow_steps.js (1)
134-190: Reduce code duplication in follower assertion stepsThe three follower assertion steps have significant code duplication. They all fetch the followers list using identical code and then perform slightly different checks on it.
Extract the common functionality to a helper function:
async function getFollowers() { const response = await fetchActivityPub( 'http://fake-ghost-activitypub.test/.ghost/activitypub/followers/index', { headers: { Accept: 'application/ld+json', }, } ); return await response.json(); } Then('{string} is in our Followers', async function (actorName) { const followers = await getFollowers(); const actor = this.actors[actorName]; const found = (followers.orderedItems || []).find( (item) => item === actor.id ); assert(found); }); // Then update the other assertion steps similarlyfeatures/step_definitions/repost_steps.js (1)
1-15: Good implementation of repost actions with a minor optimization opportunity.The repost functionality is well implemented with proper encodeURIComponent usage for URL safety. However, I notice the hardcoded base URL appears in multiple places throughout this file.
Consider extracting the base URL to a constant at the top of the file to improve maintainability:
import assert from 'node:assert'; import { Then, When } from '@cucumber/cucumber'; import { fetchActivityPub } from '../support/request.js'; +const BASE_URL = 'http://fake-ghost-activitypub.test/.ghost/activitypub'; + When('we repost the object {string}', async function (name) { const id = this.objects[name].id; this.response = await fetchActivityPub( - `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/repost/${encodeURIComponent(id)}`, + `${BASE_URL}/actions/repost/${encodeURIComponent(id)}`, { method: 'POST', }, ); });features/step_definitions/reply_steps.js (2)
47-62: Consider extracting the hardcoded test ID.The hardcoded ID for testing unknown objects could be better defined as a constant at the top of the file.
import { Given, When } from '@cucumber/cucumber'; import { fetchActivityPub } from '../support/request.js'; +const UNKNOWN_OBJECT_ID = 'http://fake-external-activitypub.test/note/123'; + Given('{string} is a reply to {string}', async function (objectA, objectB) { this.objects[objectA].inReplyTo = this.objects[objectB].id; }); ... When('we attempt to reply to an unknown object', async function () { - const id = 'http://fake-external-activitypub.test/note/123'; + const id = UNKNOWN_OBJECT_ID; this.response = await fetchActivityPub(
64-117: DRY opportunity: Reduce duplication in reply creation steps.The two reply steps (with and without image) share a lot of common code. Consider extracting the common logic to reduce duplication.
Create a helper function for reply creation:
import { Given, When } from '@cucumber/cucumber'; import { fetchActivityPub } from '../support/request.js'; +async function createReply(context, objectName, replyName, replyContent, imageUrl = null) { + const object = context.objects[objectName]; + const body = { content: replyContent }; + + if (imageUrl) { + body.imageUrl = imageUrl; + } + + context.response = await fetchActivityPub( + `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/reply/${encodeURIComponent(object.id)}`, + { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + }, + body: JSON.stringify(body), + }, + ); + + if (context.response.ok) { + const activity = await context.response.clone().json(); + context.activities[replyName] = activity; + context.objects[replyName] = activity.object; + } +} ... When( 'we reply {string} to {string} with the content', async function (replyName, objectName, replyContent) { + await createReply(this, objectName, replyName, replyContent); - const object = this.objects[objectName]; - - this.response = await fetchActivityPub( - `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/reply/${encodeURIComponent(object.id)}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - content: replyContent, - }), - }, - ); - - if (this.response.ok) { - const activity = await this.response.clone().json(); - - this.activities[replyName] = activity; - this.objects[replyName] = activity.object; - } }, ); When( 'we reply {string} to {string} with imageUrl {string} and content', async function (replyName, objectName, imageUrl, replyContent) { + await createReply(this, objectName, replyName, replyContent, imageUrl); - const object = this.objects[objectName]; - - this.response = await fetchActivityPub( - `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/reply/${encodeURIComponent(object.id)}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ - content: replyContent, - imageUrl: imageUrl, - }), - }, - ); - - if (this.response.ok) { - const activity = await this.response.clone().json(); - - this.activities[replyName] = activity; - this.objects[replyName] = activity.object; - } }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
features/step_definitions/account_steps.js(1 hunks)features/step_definitions/activitypub_steps.js(1 hunks)features/step_definitions/feed_steps.js(1 hunks)features/step_definitions/follow_steps.js(1 hunks)features/step_definitions/index.js(1 hunks)features/step_definitions/like_steps.js(1 hunks)features/step_definitions/note_steps.js(1 hunks)features/step_definitions/post_steps.js(1 hunks)features/step_definitions/reply_steps.js(1 hunks)features/step_definitions/repost_steps.js(1 hunks)features/step_definitions/request_steps.js(1 hunks)features/step_definitions/site_steps.js(1 hunks)features/step_definitions/stepdefs.js(0 hunks)features/step_definitions/thread_steps.js(1 hunks)features/step_definitions/webhook_steps.js(1 hunks)features/support/activitypub.js(1 hunks)features/support/fixtures.js(2 hunks)features/support/request.js(1 hunks)
💤 Files with no reviewable changes (1)
- features/step_definitions/stepdefs.js
🧰 Additional context used
🧬 Code Graph Analysis (7)
features/step_definitions/thread_steps.js (8)
features/step_definitions/request_steps.js (3)
objectName(26-26)responseJson(130-130)object(28-29)features/step_definitions/feed_steps.js (3)
responseJson(8-8)responseJson(24-24)object(26-26)features/step_definitions/account_steps.js (2)
responseJson(6-6)responseJson(24-24)features/step_definitions/post_steps.js (10)
responseJson(8-8)responseJson(31-31)responseJson(52-52)responseJson(64-64)responseJson(80-80)object(10-10)object(33-33)object(66-66)post(67-67)post(81-81)features/step_definitions/like_steps.js (2)
object(37-37)object(54-54)features/step_definitions/reply_steps.js (4)
object(12-12)object(30-30)object(67-67)object(94-94)features/step_definitions/repost_steps.js (3)
object(27-27)object(46-46)object(74-74)features/step_definitions/activitypub_steps.js (1)
object(23-27)
features/step_definitions/site_steps.js (2)
features/support/db.js (1)
getClient(6-22)features/support/request.js (1)
fetchActivityPub(13-40)
features/step_definitions/like_steps.js (4)
features/step_definitions/reply_steps.js (5)
id(48-48)object(12-12)object(30-30)object(67-67)object(94-94)features/step_definitions/repost_steps.js (14)
id(8-8)id(55-55)response(18-25)response(37-44)response(65-72)inbox(26-26)inbox(45-45)inbox(73-73)object(27-27)object(46-46)object(74-74)found(29-29)found(48-48)found(75-75)features/support/request.js (2)
fetchActivityPub(13-40)found(52-52)features/step_definitions/activitypub_steps.js (12)
response(453-460)inbox(337-337)inbox(366-366)inbox(461-461)object(23-27)object(54-58)found(247-247)found(271-271)found(309-311)found(340-346)found(368-379)found(395-395)
features/support/activitypub.js (10)
features/step_definitions/feed_steps.js (2)
activity(25-25)object(26-26)features/step_definitions/note_steps.js (3)
activity(52-52)activity(78-78)activity(89-89)features/step_definitions/follow_steps.js (3)
activity(102-102)object(101-101)response(105-111)features/step_definitions/reply_steps.js (6)
activity(83-83)activity(111-111)object(12-12)object(30-30)object(67-67)object(94-94)features/step_definitions/post_steps.js (6)
activity(9-9)activity(32-32)activity(65-65)object(10-10)object(33-33)object(66-66)features/step_definitions/activitypub_steps.js (12)
activity(29-29)activity(60-60)activity(109-109)activity(136-136)activity(270-270)activity(279-279)MAX_RETRIES(246-246)MAX_RETRIES(291-291)response(453-460)inbox(337-337)inbox(366-366)inbox(461-461)features/step_definitions/like_steps.js (6)
object(37-37)object(54-54)response(28-35)response(45-52)inbox(36-36)inbox(53-53)features/step_definitions/repost_steps.js (9)
object(27-27)object(46-46)object(74-74)response(18-25)response(37-44)response(65-72)inbox(26-26)inbox(45-45)inbox(73-73)features/step_definitions/request_steps.js (1)
object(28-29)features/support/request.js (1)
fetchActivityPub(13-40)
features/step_definitions/post_steps.js (6)
features/step_definitions/feed_steps.js (5)
responseJson(8-8)responseJson(24-24)activity(25-25)object(26-26)found(27-27)features/step_definitions/account_steps.js (2)
responseJson(6-6)responseJson(24-24)features/step_definitions/request_steps.js (2)
responseJson(130-130)object(28-29)features/step_definitions/thread_steps.js (4)
responseJson(8-8)responseJson(23-23)object(10-10)post(11-11)features/step_definitions/activitypub_steps.js (18)
activity(29-29)activity(60-60)activity(109-109)activity(136-136)activity(270-270)activity(279-279)activity(338-338)activity(362-362)activity(437-437)object(23-27)object(54-58)object(102-102)found(247-247)found(271-271)found(309-311)found(340-346)found(368-379)found(395-395)features/step_definitions/like_steps.js (4)
object(37-37)object(54-54)found(39-39)found(56-56)
features/step_definitions/request_steps.js (8)
features/support/request.js (2)
__dirname(11-11)fetchActivityPub(13-40)features/step_definitions/feed_steps.js (1)
object(26-26)features/step_definitions/like_steps.js (2)
object(37-37)object(54-54)features/step_definitions/reply_steps.js (4)
object(12-12)object(30-30)object(67-67)object(94-94)features/step_definitions/repost_steps.js (3)
object(27-27)object(46-46)object(74-74)features/step_definitions/thread_steps.js (1)
object(10-10)features/step_definitions/post_steps.js (3)
object(10-10)object(33-33)object(66-66)features/step_definitions/activitypub_steps.js (1)
object(23-27)
features/step_definitions/webhook_steps.js (2)
features/support/fixtures.js (2)
createWebhookPost(396-420)getWebhookSecret(424-433)features/support/request.js (1)
fetchActivityPub(13-40)
🔇 Additional comments (38)
features/support/fixtures.js (3)
1-4: Good use of Node.js core module imports with proper namespace.The code correctly imports Node.js core modules with the
node:prefix, which is a good practice for clarity and to distinguish them from third-party modules.
9-10: Correctly defines ES module equivalents for CommonJS globals.Since this is an ES module, the code properly defines
__filenameand__dirnamevariables which aren't available by default in ESM.
422-433: Well-implemented caching pattern for webhook secret.The implementation follows a good singleton pattern that:
- Caches the webhook secret to avoid repeated file reads
- Uses proper path resolution with
resolve(__dirname, ...)- Provides a clean interface for other modules to access the secret
This approach is efficient and follows good practices for handling fixture data.
features/support/request.js (1)
4-4: Minor formatting improvement.Added consistent blank line after imports to match the style used throughout the codebase.
features/step_definitions/site_steps.js (3)
1-5: Well-structured imports for step definition file.The imports correctly bring in Cucumber step definition functions and the necessary support utilities.
6-10: Properly implemented database setup step.This Given step:
- Clears the sites table to ensure tests start from a known state
- Resets the context's SITE_ID to null
- Follows the single responsibility principle by handling just database setup
This provides a clean foundation for site-related tests.
12-21: Clean implementation of site endpoint request.This When step:
- Makes a proper ActivityPub request with appropriate headers
- Stores the response in the test context for later assertions
- Uses the shared
fetchActivityPubutility consistently with other stepsThe hardcoded URL is consistent with the testing approach used throughout the codebase.
features/step_definitions/thread_steps.js (3)
1-4: Appropriate imports for thread assertion steps.The code correctly imports Node.js assert module and Cucumber's Then function for creating assertion steps.
5-20: Well-implemented post position verification in thread.This step:
- Properly retrieves and parses the JSON response
- Gets the referenced object from the test context
- Extracts the post at the specified position (with 1-based indexing)
- Performs appropriate assertions with descriptive error messages
- Compares URLs/IDs to verify post identity
The implementation is consistent with the patterns used in other step files.
22-30: Clear and concise thread size verification.This step:
- Correctly parses the JSON response
- Performs a simple but effective count verification
- Provides a detailed assertion message that includes both expected and actual counts
The implementation is straightforward and follows the established patterns in the test suite.
features/step_definitions/like_steps.js (1)
7-15: LGTM: Like operation implementation works correctlyThe implementation correctly retrieves the object ID from test context and makes a POST request to the ActivityPub like action endpoint.
features/step_definitions/repost_steps.js (7)
1-15: Well-structured and concise repost implementation.The repost step definition follows best practices by properly encoding the object ID in the URL path and using appropriate HTTP methods for the operation.
17-32: Good verification approach for repost status.This step correctly fetches the inbox data and verifies the repost status using assertions. The code pattern aligns with other verification steps in the test suite.
34-52: Effective implementation for repost count verification.The step definition follows the same pattern as the repost status check, properly asserting the exact repost count value.
54-78: Clear implementation of repost undoing and verification.The step definitions for undoing a repost and verifying an object is not reposted follow consistent patterns and use appropriate assertions.
17-32: LGTM: Effective verification of repost state.The implementation correctly fetches the inbox data and verifies the repost state by finding the matching object and checking its reposted property.
34-52: Well-structured assertion for repost count.The step definition follows good practices for asynchronous code handling and assertion checking.
54-62: LGTM: Clear implementation for undoing reposts.The implementation is straightforward and follows the same pattern as the repost action.
features/step_definitions/reply_steps.js (7)
5-7: Simple and effective reply relationship setup.This step clearly establishes the reply relationship by setting the
inReplyToproperty on the object.
9-45: Well-structured error case handling for replies.These step definitions correctly test error cases for replies, including attempts with no content and invalid content. The implementation follows good practices for API testing.
47-62: Good test for handling unknown objects.This step properly tests the scenario of attempting to reply to an unknown object, which is an important edge case to verify.
64-89: Effective implementation for creating valid replies.The step correctly handles the reply creation process and stores the response data in the test context for later verification, which is essential for maintaining test state.
91-117: Complete handling of replies with image attachments.This step definition properly extends the basic reply functionality to include image URLs, following the same pattern as the text-only replies.
1-8: LGTM: Clean setup with appropriate imports.The imports are clear and the "is a reply to" step definition properly establishes a relationship between objects.
9-45: Well-structured test cases for error scenarios.Good implementation of test cases for attempting to create replies with invalid or missing content. The error cases are important to ensure robust handling of bad inputs.
features/step_definitions/note_steps.js (4)
7-33: Clear test cases for note creation error scenarios.These steps appropriately test the error cases for note creation with no content and invalid content.
35-58: Clean implementation for creating notes with content.The step definition properly handles note creation with content and stores the response data in the test context, following the established pattern used in other step files.
60-84: Well-structured implementation for notes with images.This step correctly extends the basic note creation to include image URLs, maintaining consistency with the text-only note creation pattern.
1-34: LGTM: Well-structured test cases for note creation error handling.The implementation correctly tests scenarios where note creation should fail due to invalid or missing content, which is important for validating error handling.
features/step_definitions/request_steps.js (6)
13-49: Good implementation of dynamic path resolution for authenticated requests.The step definition handles complex scenarios where object IDs need to be resolved dynamically based on the test context. The implementation is robust and follows the established patterns in the test suite.
51-66: Clear handling of authenticated requests with JSON data.This step definition properly handles requests with JSON data, converting the data table to a JSON object and sending it with the appropriate content type.
89-99: Clear implementation of unauthenticated requests.This step correctly passes
falseas the third parameter tofetchActivityPubto make an unauthenticated request, which aligns with the function's implementation in the support code.
101-127: Well-structured response status verification steps.These step definitions provide clear assertions with helpful error messages when verifying response statuses, which will make test failures more informative.
129-140: Thorough validation of file URL responses.This step performs multiple validations on the file URL property, ensuring it exists, is a string, and has a valid format, which provides comprehensive verification.
1-12: LGTM: Appropriate imports and module setup.The imports are relevant to the file's purpose and the ESM module path handling is correctly implemented.
features/step_definitions/follow_steps.js (3)
32-66: Inconsistent behavior between follow step definitionsThe step
we are following {string}performs a complete follow interaction including creating Accept activities and waiting for inbox processing, whilewe follow {string}only sends the follow request without waiting. This inconsistency could lead to confusion when writing tests.Consider whether these steps should be consistent in their behavior or if the different names clearly indicate the different behaviors. If they should be consistent, update the simpler step to match the behavior of the more complete one.
Also applies to: 68-83
1-9: LGTM: Imports are well-organizedThe imports are well-organized, with standard library imports first, followed by package imports, and then local imports. This follows good import organization practices.
125-132: LGTM: Clean implementation of data table handlingNice implementation of the data table handling for creating multiple followers at once. This makes test writing more concise when setting up multiple followers.
| const found = inbox.items.find((item) => item.object.id === object.id); | ||
|
|
||
| assert(found.object.liked === true); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling case when object is not found in inbox
The assertion will throw an error if found is undefined (when the object isn't found in the inbox). Consider adding a more descriptive error message.
- assert(found.object.liked === true);
+ assert(found, `Object with ID ${object.id} not found in inbox`);
+ assert(found.object.liked === true, `Object is not liked as expected`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const found = inbox.items.find((item) => item.object.id === object.id); | |
| assert(found.object.liked === true); | |
| const found = inbox.items.find((item) => item.object.id === object.id); | |
| assert(found, `Object with ID ${object.id} not found in inbox`); | |
| assert(found.object.liked === true, `Object is not liked as expected`); |
| const found = inbox.items.find((item) => item.object.id === object.id); | ||
|
|
||
| assert(found.object.liked !== true); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling case when object is not found in inbox
Similar to the previous step, handle the case when the object isn't found in the inbox with a descriptive error message.
- assert(found.object.liked !== true);
+ assert(found, `Object with ID ${object.id} not found in inbox`);
+ assert(found.object.liked !== true, `Object is liked but should not be`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const found = inbox.items.find((item) => item.object.id === object.id); | |
| assert(found.object.liked !== true); | |
| const found = inbox.items.find((item) => item.object.id === object.id); | |
| assert(found, `Object with ID ${object.id} not found in inbox`); | |
| assert(found.object.liked !== true, `Object is liked but should not be`); |
| When('we request the feed with the next cursor', async function () { | ||
| const responseJson = await this.response.clone().json(); | ||
| const nextCursor = responseJson.next; | ||
|
|
||
| this.response = await fetchActivityPub( | ||
| `http://fake-ghost-activitypub.test/.ghost/activitypub/feed/index?next=${encodeURIComponent(nextCursor)}`, | ||
| { | ||
| headers: { | ||
| Accept: 'application/json', | ||
| }, | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for next cursor and HTTP response
Consider adding checks for the existence of nextCursor and for the HTTP response status before proceeding.
When('we request the feed with the next cursor', async function () {
const responseJson = await this.response.clone().json();
const nextCursor = responseJson.next;
+
+ if (!nextCursor) {
+ throw new Error('No next cursor available in the response');
+ }
this.response = await fetchActivityPub(
`http://fake-ghost-activitypub.test/.ghost/activitypub/feed/index?next=${encodeURIComponent(nextCursor)}`,
{
headers: {
Accept: 'application/json',
},
},
);
+
+ if (!this.response.ok) {
+ throw new Error(`Failed to fetch feed: ${this.response.status} ${this.response.statusText}`);
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When('we request the feed with the next cursor', async function () { | |
| const responseJson = await this.response.clone().json(); | |
| const nextCursor = responseJson.next; | |
| this.response = await fetchActivityPub( | |
| `http://fake-ghost-activitypub.test/.ghost/activitypub/feed/index?next=${encodeURIComponent(nextCursor)}`, | |
| { | |
| headers: { | |
| Accept: 'application/json', | |
| }, | |
| }, | |
| ); | |
| }); | |
| When('we request the feed with the next cursor', async function () { | |
| const responseJson = await this.response.clone().json(); | |
| const nextCursor = responseJson.next; | |
| if (!nextCursor) { | |
| throw new Error('No next cursor available in the response'); | |
| } | |
| this.response = await fetchActivityPub( | |
| `http://fake-ghost-activitypub.test/.ghost/activitypub/feed/index?next=${encodeURIComponent(nextCursor)}`, | |
| { | |
| headers: { | |
| Accept: 'application/json', | |
| }, | |
| }, | |
| ); | |
| if (!this.response.ok) { | |
| throw new Error(`Failed to fetch feed: ${this.response.status} ${this.response.statusText}`); | |
| } | |
| }); |
| Then( | ||
| 'the {string} in the feed has content {string}', | ||
| async function (activityOrObjectName, content) { | ||
| const responseJson = await this.response.clone().json(); | ||
| const activity = this.activities[activityOrObjectName]; | ||
| const object = this.objects[activityOrObjectName]; | ||
| let found; | ||
|
|
||
| if (activity) { | ||
| found = responseJson.posts.find( | ||
| (post) => post.url === activity.object.id, | ||
| ); | ||
| } else if (object) { | ||
| found = responseJson.posts.find((post) => post.url === object.id); | ||
| } | ||
|
|
||
| assert.equal(found.content, content); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for object not found in feed
The current implementation doesn't handle the case where the activity or object isn't found in the feed. Add appropriate error handling.
Then(
'the {string} in the feed has content {string}',
async function (activityOrObjectName, content) {
const responseJson = await this.response.clone().json();
const activity = this.activities[activityOrObjectName];
const object = this.objects[activityOrObjectName];
let found;
if (activity) {
found = responseJson.posts.find(
(post) => post.url === activity.object.id,
);
} else if (object) {
found = responseJson.posts.find((post) => post.url === object.id);
}
+ if (!found) {
+ throw new Error(`Activity or object '${activityOrObjectName}' not found in feed`);
+ }
assert.equal(found.content, content);
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Then( | |
| 'the {string} in the feed has content {string}', | |
| async function (activityOrObjectName, content) { | |
| const responseJson = await this.response.clone().json(); | |
| const activity = this.activities[activityOrObjectName]; | |
| const object = this.objects[activityOrObjectName]; | |
| let found; | |
| if (activity) { | |
| found = responseJson.posts.find( | |
| (post) => post.url === activity.object.id, | |
| ); | |
| } else if (object) { | |
| found = responseJson.posts.find((post) => post.url === object.id); | |
| } | |
| assert.equal(found.content, content); | |
| }, | |
| ); | |
| Then( | |
| 'the {string} in the feed has content {string}', | |
| async function (activityOrObjectName, content) { | |
| const responseJson = await this.response.clone().json(); | |
| const activity = this.activities[activityOrObjectName]; | |
| const object = this.objects[activityOrObjectName]; | |
| let found; | |
| if (activity) { | |
| found = responseJson.posts.find( | |
| (post) => post.url === activity.object.id, | |
| ); | |
| } else if (object) { | |
| found = responseJson.posts.find((post) => post.url === object.id); | |
| } | |
| if (!found) { | |
| throw new Error(`Activity or object '${activityOrObjectName}' not found in feed`); | |
| } | |
| assert.equal(found.content, content); | |
| }, | |
| ); |
| const payload = createWebhookPost(); | ||
| const body = JSON.stringify(payload); | ||
| const timestamp = Date.now() - 60 * 60 * 1000; // An hour old | ||
| const hmac = createHmac('sha256', getWebhookSecret()) | ||
| .update(body + timestamp) | ||
| .digest('hex'); | ||
|
|
||
| this.response = await fetchActivityPub(endpoint, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'X-Ghost-Signature': `sha256=${hmac}, t=${timestamp}`, | ||
| }, | ||
| body: body, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
payloadData customisations are lost in the “old signature” path
The “old signature” step builds a fresh payload but forgets to merge in the scenario-specific overrides set in the previous Given step. Assertions against modified fields will therefore fail.
- const payload = createWebhookPost();
+ let payload = createWebhookPost();
+ if (this.payloadData) {
+ payload = merge(payload, this.payloadData);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload = createWebhookPost(); | |
| const body = JSON.stringify(payload); | |
| const timestamp = Date.now() - 60 * 60 * 1000; // An hour old | |
| const hmac = createHmac('sha256', getWebhookSecret()) | |
| .update(body + timestamp) | |
| .digest('hex'); | |
| this.response = await fetchActivityPub(endpoint, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'X-Ghost-Signature': `sha256=${hmac}, t=${timestamp}`, | |
| }, | |
| body: body, | |
| }); | |
| }, | |
| // build an hour-old signature, preserving any scenario-specific overrides | |
| - const payload = createWebhookPost(); | |
| + let payload = createWebhookPost(); | |
| + if (this.payloadData) { | |
| + payload = merge(payload, this.payloadData); | |
| + } | |
| const body = JSON.stringify(payload); | |
| const timestamp = Date.now() - 60 * 60 * 1000; // An hour old | |
| const hmac = createHmac('sha256', getWebhookSecret()) | |
| .update(body + timestamp) | |
| .digest('hex'); | |
| this.response = await fetchActivityPub(endpoint, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| 'X-Ghost-Signature': `sha256=${hmac}, t=${timestamp}`, | |
| }, | |
| body: body, | |
| }); | |
| }, |
| async function getActor(input) { | ||
| const existingActor = this.actors[input]; | ||
|
|
||
| let type = 'Person'; | ||
| let name = input; | ||
|
|
||
| if (!existingActor) { | ||
| const parsed = parseActorString(input); | ||
| if (parsed.type && parsed.name) { | ||
| type = parsed.type; | ||
| name = parsed.name; | ||
| } | ||
| this.actors[name] = await createActor(name, { type }); | ||
| } | ||
|
|
||
| return { | ||
| type, | ||
| name, | ||
| actor: this.actors[name], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent type handling in getActor function
The function correctly parses actor type from input strings, but has a potential issue with returning the correct type for existing actors. When an actor already exists in this.actors, the function still initializes type to 'Person' but doesn't update it based on the existing actor's actual type.
Consider modifying the function to extract type information from existing actors:
async function getActor(input) {
const existingActor = this.actors[input];
let type = 'Person';
let name = input;
if (!existingActor) {
const parsed = parseActorString(input);
if (parsed.type && parsed.name) {
type = parsed.type;
name = parsed.name;
}
this.actors[name] = await createActor(name, { type });
+ } else {
+ // Extract type from existing actor if possible
+ type = existingActor.type || type;
}
return {
type,
name,
actor: this.actors[name],
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function getActor(input) { | |
| const existingActor = this.actors[input]; | |
| let type = 'Person'; | |
| let name = input; | |
| if (!existingActor) { | |
| const parsed = parseActorString(input); | |
| if (parsed.type && parsed.name) { | |
| type = parsed.type; | |
| name = parsed.name; | |
| } | |
| this.actors[name] = await createActor(name, { type }); | |
| } | |
| return { | |
| type, | |
| name, | |
| actor: this.actors[name], | |
| }; | |
| } | |
| async function getActor(input) { | |
| const existingActor = this.actors[input]; | |
| let type = 'Person'; | |
| let name = input; | |
| if (!existingActor) { | |
| const parsed = parseActorString(input); | |
| if (parsed.type && parsed.name) { | |
| type = parsed.type; | |
| name = parsed.name; | |
| } | |
| this.actors[name] = await createActor(name, { type }); | |
| } else { | |
| // Extract type from existing actor if possible | |
| type = existingActor.type || type; | |
| } | |
| return { | |
| type, | |
| name, | |
| actor: this.actors[name], | |
| }; | |
| } |
| this.activities[`Unfollow(${name})`] = await this.response | ||
| .clone() | ||
| .json(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential JSON parsing error for non-JSON responses
The we unfollow {string} step attempts to parse the response as JSON without checking if the response actually contains JSON data. If the server returns a non-JSON response (like plain text or HTML), this will throw an error.
Add a check for the content type or handle potential parsing errors:
if (this.response.ok) {
- this.activities[`Unfollow(${name})`] = await this.response
- .clone()
- .json();
+ try {
+ this.activities[`Unfollow(${name})`] = await this.response
+ .clone()
+ .json();
+ } catch (e) {
+ // Handle case where response isn't JSON
+ this.activities[`Unfollow(${name})`] = {
+ status: 'unfollow_sent',
+ responseStatus: this.response.status
+ };
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.activities[`Unfollow(${name})`] = await this.response | |
| .clone() | |
| .json(); | |
| } | |
| if (this.response.ok) { | |
| try { | |
| this.activities[`Unfollow(${name})`] = await this.response | |
| .clone() | |
| .json(); | |
| } catch (e) { | |
| // Handle case where response isn't JSON | |
| this.activities[`Unfollow(${name})`] = { | |
| status: 'unfollow_sent', | |
| responseStatus: this.response.status | |
| }; | |
| } | |
| } |
| Then('the object {string} should not be reposted', async function (name) { | ||
| const response = await fetchActivityPub( | ||
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | ||
| { | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }, | ||
| ); | ||
| const inbox = await response.json(); | ||
| const object = this.objects[name]; | ||
| const found = inbox.items.find((item) => item.object.id === object.id); | ||
|
|
||
| assert(found.object.reposted !== true); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling potential null/undefined object.
The implementation assumes that found will always exist in the inbox items. If the object is not found, this will result in a null reference error.
Add a null check before accessing the object properties:
const inbox = await response.json();
const object = this.objects[name];
const found = inbox.items.find((item) => item.object.id === object.id);
- assert(found.object.reposted !== true);
+ assert(found && found.object.reposted !== true, 'Object not found in inbox or is still reposted');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Then('the object {string} should not be reposted', async function (name) { | |
| const response = await fetchActivityPub( | |
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | |
| { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }, | |
| ); | |
| const inbox = await response.json(); | |
| const object = this.objects[name]; | |
| const found = inbox.items.find((item) => item.object.id === object.id); | |
| assert(found.object.reposted !== true); | |
| }); | |
| Then('the object {string} should not be reposted', async function (name) { | |
| const response = await fetchActivityPub( | |
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | |
| { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }, | |
| ); | |
| const inbox = await response.json(); | |
| const object = this.objects[name]; | |
| const found = inbox.items.find((item) => item.object.id === object.id); | |
| assert( | |
| found && found.object.reposted !== true, | |
| 'Object not found in inbox or is still reposted' | |
| ); | |
| }); |
| Then( | ||
| 'note {string} has the image URL {string}', | ||
| function (activityName, expectedImageUrl) { | ||
| const activity = this.activities[activityName]; | ||
| assert.equal(activity.object.attachment.url, expectedImageUrl); | ||
| assert.equal(activity.object.attachment.type, 'Image'); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive check for attachment property.
The assertion assumes that activity.object.attachment exists and has url and type properties, but doesn't verify this structure first.
Add defensive checks:
Then(
'note {string} has the image URL {string}',
function (activityName, expectedImageUrl) {
const activity = this.activities[activityName];
+ assert(activity.object.attachment, 'Activity object should have an attachment');
+ assert(typeof activity.object.attachment === 'object', 'Attachment should be an object');
assert.equal(activity.object.attachment.url, expectedImageUrl);
assert.equal(activity.object.attachment.type, 'Image');
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Then( | |
| 'note {string} has the image URL {string}', | |
| function (activityName, expectedImageUrl) { | |
| const activity = this.activities[activityName]; | |
| assert.equal(activity.object.attachment.url, expectedImageUrl); | |
| assert.equal(activity.object.attachment.type, 'Image'); | |
| }, | |
| ); | |
| Then( | |
| 'note {string} has the image URL {string}', | |
| function (activityName, expectedImageUrl) { | |
| const activity = this.activities[activityName]; | |
| assert(activity.object.attachment, 'Activity object should have an attachment'); | |
| assert(typeof activity.object.attachment === 'object', 'Attachment should be an object'); | |
| assert.equal(activity.object.attachment.url, expectedImageUrl); | |
| assert.equal(activity.object.attachment.type, 'Image'); | |
| }, | |
| ); |
| When( | ||
| /an authenticated (\"(delete|get|post|put)\"\s)?request is made to "(.*)"$/, | ||
| async function (method, path) { | ||
| const requestMethod = method || 'get'; | ||
| let requestPath = path; | ||
|
|
||
| // If this is a request to the /thread/ or delete post endpoint, we need to replace the | ||
| // object name with the object ID as we don't have a way to know the object | ||
| // ID ahead of time | ||
| if ( | ||
| path.includes('/thread/') || | ||
| (requestMethod === 'delete' && path.includes('/post/')) | ||
| ) { | ||
| const objectName = path.split('/').pop(); // Object name is the last part of the path | ||
|
|
||
| const object = | ||
| this.objects[objectName] || this.activities[objectName]?.object; | ||
|
|
||
| if (object) { | ||
| requestPath = path.replace( | ||
| objectName, | ||
| encodeURIComponent(object.id), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| this.response = await fetchActivityPub( | ||
| `http://fake-ghost-activitypub.test${requestPath}`, | ||
| { | ||
| method: requestMethod, | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }, | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with request method default value.
The step definition pattern for the HTTP method has a potential issue. The regex captures a string like "get" (with quotes), but the default value is just 'get' (without quotes).
Fix the inconsistency in how the method is used:
When(
/an authenticated (\"(delete|get|post|put)\"\s)?request is made to "(.*)"$/,
async function (method, path) {
- const requestMethod = method || 'get';
+ // Extract the actual method from the captured group or default to 'get'
+ const requestMethod = method ? method.replace(/["\s]/g, '') : 'get';
let requestPath = path;
// ...rest of the functionAlso, the URL construction is repeated throughout the file - consider extracting the base URL as a constant.
no refs Refactored cucumber steps into separate files, grouped by either feature or functionality (for generic steps). `stepdefs.js` has been renamed to `index.js` and contains the hooks to execute as well as any global setup
mike182uk
force-pushed
the
cucumber-split-up-steps
branch
from
0f56008 to
47c4a6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
features/step_definitions/repost_steps.js (6)
75-77:⚠️ Potential issueConsider handling potential null/undefined object.
The implementation assumes that
foundwill always exist in the inbox items. If the object is not found, this will result in a null reference error.const object = this.objects[name]; const found = inbox.items.find((item) => item.object.id === object.id); - assert(found.object.reposted !== true); + assert(found && found.object.reposted !== true, 'Object not found in inbox or is still reposted');
29-31:⚠️ Potential issueAdd null check before accessing
found.object.If the object is not found in the inbox items, this will result in a TypeError when trying to access
found.object.const object = this.objects[name]; const found = inbox.items.find((item) => item.object.id === object.id); - assert(found.object.reposted === true); + assert(found && found.object.reposted === true, 'Object not found in inbox or is not reposted');
48-50:⚠️ Potential issueAdd null check before accessing
found.object.If the object is not found in the inbox items, this will result in a TypeError when trying to access
found.object.const found = inbox.items.find((item) => item.object.id === object.id); - assert(found.object.repostCount === repostCount); + assert(found && found.object.repostCount === repostCount, `Object not found in inbox or repost count doesn't match ${repostCount}`);
76-77: Consider handling potential null/undefined object.The implementation assumes that
foundwill always exist in the inbox items. If the object is not found, this will result in a null reference error.Add a null check before accessing the object properties:
const found = inbox.items.find((item) => item.object.id === object.id); - assert(found.object.reposted !== true); + assert(found && found.object.reposted !== true, 'Object not found in inbox or is still reposted');
29-31:⚠️ Potential issueHandle potential null/undefined object.
The implementation assumes that
foundwill always exist in the inbox items. If the object is not found, this will result in a null reference error.Add a null check before accessing the object properties:
const found = inbox.items.find((item) => item.object.id === object.id); - assert(found.object.reposted === true); + assert(found && found.object.reposted === true, 'Object not found in inbox or is not reposted');
48-50:⚠️ Potential issueHandle potential null/undefined object.
Similar to the issue found in other assertions, this code assumes that
foundwill always exist in the inbox items. If the object is not found, this will result in a null reference error.Add a null check before accessing the object properties:
const found = inbox.items.find((item) => item.object.id === object.id); - assert(found.object.repostCount === repostCount); + assert(found && found.object.repostCount === repostCount, `Object not found in inbox or does not have expected repost count of ${repostCount}`);
🧹 Nitpick comments (9)
features/step_definitions/activitypub_steps.js (9)
165-165: Fix typo in variable name.There's a typo in the variable name: "Reponse" should be "Response".
- const firstPageReponse = await fetchActivityPub( + const firstPageResponse = await fetchActivityPub(
229-229: Fix typo in variable name.The same typo appears in another instance.
- const firstPageReponse = await fetchActivityPub(initialResponseJson.first, { + const firstPageResponse = await fetchActivityPub(initialResponseJson.first, {
302-302: Fix typo in variable name.The same typo appears in a third instance.
- const firstPageReponse = await fetchActivityPub(initialResponseJson.first, { + const firstPageResponse = await fetchActivityPub(initialResponseJson.first, {
239-267: Consider using a loop instead of recursion for retries.Using recursion for retrying operations can lead to call stack issues and is generally less efficient than using a loop. Consider refactoring to use a while loop instead.
-async function waitForOutboxActivity( - activity, - options = { - retryCount: 0, - delay: 0, - }, -) { - const MAX_RETRIES = 5; - const found = await findInOutbox(activity); - - if (found) { - return; - } - - if (options.retryCount >= MAX_RETRIES) { - throw new Error( - `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the outbox`, - ); - } - - if (options.delay > 0) { - await new Promise((resolve) => setTimeout(resolve, options.delay)); - } - - await waitForOutboxActivity(activity, { - retryCount: options.retryCount + 1, - delay: options.delay + 500, - }); -} +async function waitForOutboxActivity(activity) { + const MAX_RETRIES = 5; + let retryCount = 0; + let delay = 0; + + while (retryCount <= MAX_RETRIES) { + const found = await findInOutbox(activity); + + if (found) { + return; + } + + if (retryCount >= MAX_RETRIES) { + throw new Error( + `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the outbox`, + ); + } + + if (delay > 0) { + await new Promise((resolve) => setTimeout(resolve, delay)); + } + + retryCount++; + delay += 500; + } +}
283-331: Consider using a loop instead of recursion for retries.Similar to the previous function, this one also uses recursion for retry logic. Consider refactoring to use a while loop for better efficiency and to avoid potential call stack issues.
-async function waitForOutboxActivityType( - activityType, - objectType, - options = { - retryCount: 0, - delay: 0, - }, -) { - const MAX_RETRIES = 5; - - const initialResponse = await fetchActivityPub( - 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', - { - headers: { - Accept: 'application/ld+json', - }, - }, - ); - const initialResponseJson = await initialResponse.json(); - const firstPageReponse = await fetchActivityPub(initialResponseJson.first, { - headers: { - Accept: 'application/ld+json', - }, - }); - const outbox = await firstPageReponse.json(); - - const found = (outbox.orderedItems || []).find((item) => { - return item.type === activityType && item.object?.type === objectType; - }); - - if (found) { - return found; - } - - if (options.retryCount === MAX_RETRIES) { - throw new Error( - `Max retries reached (${MAX_RETRIES}) when waiting for ${activityType}(${objectType}) in the outbox`, - ); - } - - if (options.delay > 0) { - await new Promise((resolve) => setTimeout(resolve, options.delay)); - } - - return waitForOutboxActivityType(activityType, objectType, { - retryCount: options.retryCount + 1, - delay: options.delay + 500, - }); -} +async function waitForOutboxActivityType(activityType, objectType) { + const MAX_RETRIES = 5; + let retryCount = 0; + let delay = 0; + + while (retryCount <= MAX_RETRIES) { + const initialResponse = await fetchActivityPub( + 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', + { + headers: { + Accept: 'application/ld+json', + }, + }, + ); + const initialResponseJson = await initialResponse.json(); + const firstPageResponse = await fetchActivityPub(initialResponseJson.first, { + headers: { + Accept: 'application/ld+json', + }, + }); + const outbox = await firstPageResponse.json(); + + const found = (outbox.orderedItems || []).find((item) => { + return item.type === activityType && item.object?.type === objectType; + }); + + if (found) { + return found; + } + + if (retryCount === MAX_RETRIES) { + throw new Error( + `Max retries reached (${MAX_RETRIES}) when waiting for ${activityType}(${objectType}) in the outbox`, + ); + } + + if (delay > 0) { + await new Promise((resolve) => setTimeout(resolve, delay)); + } + + retryCount++; + delay += 500; + } + + // This line should not be reached due to the error throw above, but TypeScript might require it + return null; +}
151-191: Consider refactoring collection fetching logic into a utility function.There's duplicated logic for fetching and parsing ActivityPub collections from various endpoints across this file. Consider extracting it into a reusable utility function to reduce code duplication.
You could create a utility function like:
// Add to support/activitypub.js export async function fetchCollection(collectionType) { const initialResponse = await fetchActivityPub( `http://fake-ghost-activitypub.test/.ghost/activitypub/${collectionType}/index`, { headers: { Accept: 'application/ld+json', }, }, ); const initialResponseJson = await initialResponse.json(); let collection = initialResponseJson; if (initialResponseJson.first) { const firstPageResponse = await fetchActivityPub( initialResponseJson.first, { headers: { Accept: 'application/ld+json', }, }, ); collection = await firstPageResponse.json(); } return collection; }Then use it in your step definitions:
import { fetchCollection } from '../support/activitypub.js'; async function getObjectInCollection(objectName, collectionType) { const collection = await fetchCollection(collectionType); const object = this.objects[objectName] || this.actors[objectName]; if (!object) { throw new Error(`Could not find object or actor with name "${objectName}"`); } return (collection.orderedItems || []).find((item) => { // existing logic }); }
246-251: Consider adding a null/undefined check for activity.The function is checking if the activity was found but doesn't verify if the activity parameter itself is valid before calling
findInOutbox.Add a check at the beginning of the function to ensure the activity parameter is valid:
async function waitForOutboxActivity( activity, options = { retryCount: 0, delay: 0, }, ) { const MAX_RETRIES = 5; + if (!activity) { + throw new Error('Activity parameter is undefined or null'); + } const found = await findInOutbox(activity); if (found) { return; }
176-191: Add validation for object existence.The function doesn't verify if
objectexists after attempting to retrieve it, which could lead to errors if a non-existent object name is provided.Add a check to ensure the object exists before proceeding:
const object = this.objects[objectName] || this.actors[objectName]; + if (!object) { + return null; + } return (collection.orderedItems || []).find((item) => {
253-267: Consider extracting the MAX_RETRIES value to a configurable constant.The MAX_RETRIES value is hardcoded in multiple places within the file. This makes it harder to maintain and change if requirements change.
Extract it to a constant at the top level of the file or to a shared configuration:
import { waitForInboxActivity } from '../support/activitypub.js'; import { createActivity, createActor, createObject, } from '../support/fixtures.js'; import { fetchActivityPub } from '../support/request.js'; import { waitForRequest } from '../support/request.js'; import { parseActivityString, parseActorString } from '../support/steps.js'; +// Configuration +const MAX_RETRY_ATTEMPTS = 5; async function activityCreatedBy(activityDef, name, actorName) {And then update all occurrences:
if (options.retryCount >= MAX_RETRIES) { throw new Error( - `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the outbox`, + `Max retries reached (${MAX_RETRY_ATTEMPTS}) when waiting on an activity in the outbox`, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
features/step_definitions/account_steps.js(1 hunks)features/step_definitions/activitypub_steps.js(1 hunks)features/step_definitions/feed_steps.js(1 hunks)features/step_definitions/follow_steps.js(1 hunks)features/step_definitions/index.js(1 hunks)features/step_definitions/like_steps.js(1 hunks)features/step_definitions/note_steps.js(1 hunks)features/step_definitions/post_steps.js(1 hunks)features/step_definitions/reply_steps.js(1 hunks)features/step_definitions/repost_steps.js(1 hunks)features/step_definitions/request_steps.js(1 hunks)features/step_definitions/site_steps.js(1 hunks)features/step_definitions/stepdefs.js(0 hunks)features/step_definitions/thread_steps.js(1 hunks)features/step_definitions/webhook_steps.js(1 hunks)features/support/activitypub.js(1 hunks)features/support/fixtures.js(2 hunks)features/support/path.js(1 hunks)features/support/request.js(1 hunks)
💤 Files with no reviewable changes (1)
- features/step_definitions/stepdefs.js
✅ Files skipped from review due to trivial changes (2)
- features/support/path.js
- features/step_definitions/index.js
🚧 Files skipped from review as they are similar to previous changes (14)
- features/support/request.js
- features/step_definitions/site_steps.js
- features/step_definitions/thread_steps.js
- features/step_definitions/feed_steps.js
- features/support/fixtures.js
- features/support/activitypub.js
- features/step_definitions/reply_steps.js
- features/step_definitions/like_steps.js
- features/step_definitions/account_steps.js
- features/step_definitions/request_steps.js
- features/step_definitions/post_steps.js
- features/step_definitions/note_steps.js
- features/step_definitions/webhook_steps.js
- features/step_definitions/follow_steps.js
| collection = await firstPageReponse.json(); | ||
| } | ||
|
|
||
| const object = this.objects[objectName] || this.actors[objectName]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add check for object existence.
The function assumes that the object exists in either this.objects or this.actors, but there's no check for this case. If the object is not found, this could lead to unexpected behavior.
const object = this.objects[objectName] || this.actors[objectName];
+
+ if (!object) {
+ throw new Error(`Could not find object or actor with name "${objectName}"`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const object = this.objects[objectName] || this.actors[objectName]; | |
| const object = this.objects[objectName] || this.actors[objectName]; | |
| if (!object) { | |
| throw new Error(`Could not find object or actor with name "${objectName}"`); | |
| } |
| if (!this.found) { | ||
| this.found = {}; | ||
| } | ||
| this.found[string] = found; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Initialize this.found at test context creation.
The object this.found is conditionally initialized in this step, which could cause issues if other parts of the code try to access it before this condition is executed. Consider initializing it when the test context is created.
🏁 Script executed:
#!/bin/bash
# Check if 'this.found' is initialized elsewhere in a World or index.js file
echo "Checking for 'this.found' initialization in index.js or world.js files..."
fd -g "index.js" -g "world.js" -p features/step_definitions | xargs grep -l "this.found" 2>/dev/null || echo "Not found in world or index files"
echo "\nChecking for access to 'this.found' in other step files..."
rg "this\.found" features/step_definitions/ | grep -v "if (!this.found)"Length of output: 1000
🏁 Script executed:
#!/bin/bash
# Locate any world or index files in the repository
echo "Searching for world.js or index.js across the repo..."
fd -g "world.js" -g "index.js"
echo -e "\nSearching for Cucumber world constructor usage (setWorldConstructor)..."
rg "setWorldConstructor" -g "*.js" || echo "No world constructor found"
echo -e "\nSearching for Before hooks in features/support..."
rg "Before\(" -p features/support || echo "No Before hooks found in features/support"Length of output: 742
Initialize this.found for every scenario
To avoid conditional setup in individual steps and ensure this.found is always available, move its initialization into a Cucumber World constructor or a Before hook.
Suggested changes:
-
Create (or update) a support file, e.g.
features/support/hooks.jsorfeatures/support/world.js -
Add:
const { Before } = require('@cucumber/cucumber'); Before(function () { this.found = {}; });
-
In
features/step_definitions/activitypub_steps.js, you can then remove theif (!this.found)guard sincethis.foundwill always be defined.
This ensures this.found is reset and available at the start of each scenario.
| if (options.retryCount === MAX_RETRIES) { | ||
| throw new Error( | ||
| `Max retries reached (${MAX_RETRIES}) when waiting for ${activityType}(${objectType}) in the outbox`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent retry condition check.
There's an inconsistency in how the retry count is checked. On line 253, the condition is options.retryCount >= MAX_RETRIES but here it's using strict equality. This could lead to different behavior between the two retry mechanisms.
Make the condition check consistent with the pattern used in the waitForOutboxActivity function:
- if (options.retryCount === MAX_RETRIES) {
+ if (options.retryCount >= MAX_RETRIES) {
throw new Error(
`Max retries reached (${MAX_RETRIES}) when waiting for ${activityType}(${objectType}) in the outbox`,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options.retryCount === MAX_RETRIES) { | |
| throw new Error( | |
| `Max retries reached (${MAX_RETRIES}) when waiting for ${activityType}(${objectType}) in the outbox`, | |
| ); | |
| } | |
| if (options.retryCount >= MAX_RETRIES) { | |
| throw new Error( | |
| `Max retries reached (${MAX_RETRIES}) when waiting for ${activityType}(${objectType}) in the outbox`, | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
features/step_definitions/repost_steps.js (1)
68-82: PotentialTypeErrorwhenfoundis undefined
A previous review already pointed this out; the risk still exists.features/step_definitions/activitypub_steps.js (1)
271-276: Make retry condition consistent (>=)This mirrors the inbox helper; using
===here is inconsistent and may under-count retries ifMAX_RETRIESis decreased.
🧹 Nitpick comments (2)
features/support/activitypub.js (1)
43-51: Cap exponential back-off delayDelay increases un-boundedly: 0 ms → 500 ms → 1 s → 1.5 s → 2 s → 2.5 s…
IfMAX_RETRIESis ever raised, waits could become excessive. Consider capping or switching to a fixed delay.features/step_definitions/activitypub_steps.js (1)
155-195: Guard against missing objects before dereferencing
getObjectInCollectionsilently proceeds whenobjectisundefined. Down-stream,object.idwill throw.- const object = this.objects[objectName] || this.actors[objectName]; + const object = + this.objects[objectName] || this.actors[objectName] || null; + + if (!object) { + throw new Error(`Unknown object or actor "${objectName}"`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/step_definitions/activitypub_steps.js(1 hunks)features/step_definitions/repost_steps.js(1 hunks)features/support/activitypub.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (1)
features/step_definitions/repost_steps.js (1)
110-128:this.objects.Notemay be undefined – please verifyHard-coding
'Note'assumes such an object was stored under that exact key in every scenario.
If the feature uses a different alias,objectwill beundefinedand the matching logic will degrade to a type-check only.Please confirm the intent or fall back to
objectNameOrTypeas is done elsewhere.
| const response = await fetchActivityPub( | ||
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | ||
| { | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }, | ||
| ); | ||
| const inbox = await response.json(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check response.ok before parsing JSON
fetchActivityPub will happily return a non-2xx response that still yields a body.
Blindly calling response.json() may mask HTTP errors and make test failures hard to interpret.
- const inbox = await response.json();
+ if (!response.ok) {
+ throw new Error(
+ `Inbox request failed: ${response.status} ${response.statusText}`,
+ );
+ }
+ const inbox = await response.json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetchActivityPub( | |
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | |
| { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }, | |
| ); | |
| const inbox = await response.json(); | |
| const response = await fetchActivityPub( | |
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | |
| { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }, | |
| ); | |
| if (!response.ok) { | |
| throw new Error( | |
| `Inbox request failed: ${response.status} ${response.statusText}`, | |
| ); | |
| } | |
| const inbox = await response.json(); |
| if (options.retryCount === MAX_RETRIES) { | ||
| throw new Error( | ||
| `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the inbox`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Retry guard should be >= not ===
The same helper later uses >=. Using strict equality here means an extra retry might sneak through when MAX_RETRIES changes.
- if (options.retryCount === MAX_RETRIES) {
+ if (options.retryCount >= MAX_RETRIES) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options.retryCount === MAX_RETRIES) { | |
| throw new Error( | |
| `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the inbox`, | |
| ); | |
| } | |
| if (options.retryCount >= MAX_RETRIES) { | |
| throw new Error( | |
| `Max retries reached (${MAX_RETRIES}) when waiting on an activity in the inbox`, | |
| ); | |
| } |
| export async function findInOutbox(activity) { | ||
| const initialResponse = await fetchActivityPub( | ||
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', | ||
| { | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }, | ||
| ); | ||
| const initialResponseJson = await initialResponse.json(); | ||
| const firstPageReponse = await fetchActivityPub(initialResponseJson.first, { | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }); | ||
| const outbox = await firstPageReponse.json(); | ||
|
|
||
| return (outbox.orderedItems || []).find((item) => item.id === activity.id); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Minor issues in findInOutbox
- Typo:
firstPageReponse→firstPageResponse. - No
response.okchecks (same concern as above). - Only the first page is inspected; large outboxes paginate. Either iterate through
nextlinks or document the limitation.
- const firstPageReponse = await fetchActivityPub(initialResponseJson.first, {
+ const firstPageResponse = await fetchActivityPub(initialResponseJson.first, {
headers: { Accept: 'application/ld+json' },
});
- const outbox = await firstPageReponse.json();
+ if (!firstPageResponse.ok) {
+ throw new Error(`Outbox request failed: ${firstPageResponse.status}`);
+ }
+ const outbox = await firstPageResponse.json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function findInOutbox(activity) { | |
| const initialResponse = await fetchActivityPub( | |
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', | |
| { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }, | |
| ); | |
| const initialResponseJson = await initialResponse.json(); | |
| const firstPageReponse = await fetchActivityPub(initialResponseJson.first, { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }); | |
| const outbox = await firstPageReponse.json(); | |
| return (outbox.orderedItems || []).find((item) => item.id === activity.id); | |
| export async function findInOutbox(activity) { | |
| const initialResponse = await fetchActivityPub( | |
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', | |
| { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }, | |
| ); | |
| const initialResponseJson = await initialResponse.json(); | |
| const firstPageResponse = await fetchActivityPub(initialResponseJson.first, { | |
| headers: { | |
| Accept: 'application/ld+json', | |
| }, | |
| }); | |
| if (!firstPageResponse.ok) { | |
| throw new Error(`Outbox request failed: ${firstPageResponse.status}`); | |
| } | |
| const outbox = await firstPageResponse.json(); | |
| return (outbox.orderedItems || []).find((item) => item.id === activity.id); | |
| } |
| When('we undo the repost of the object {string}', async function (name) { | ||
| const id = this.objects[name].id; | ||
| this.response = await fetchActivityPub( | ||
| `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/derepost/${encodeURIComponent(id)}`, | ||
| { | ||
| method: 'POST', | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same missing assertion for the undo-repost endpoint
Mirror the check suggested above to ensure failures surface.
| const id = this.objects[name].id; | ||
| this.response = await fetchActivityPub( | ||
| `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/repost/${encodeURIComponent(id)}`, | ||
| { | ||
| method: 'POST', | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert the POST outcome
The test records this.response but never asserts it. A 500 response would silently pass.
- this.response = await fetchActivityPub( … );
+ this.response = await fetchActivityPub( … );
+ assert(
+ this.response.ok,
+ `Repost request failed: ${this.response.status} ${this.response.statusText}`,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const id = this.objects[name].id; | |
| this.response = await fetchActivityPub( | |
| `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/repost/${encodeURIComponent(id)}`, | |
| { | |
| method: 'POST', | |
| }, | |
| ); | |
| }); | |
| const id = this.objects[name].id; | |
| this.response = await fetchActivityPub( | |
| `http://fake-ghost-activitypub.test/.ghost/activitypub/actions/repost/${encodeURIComponent(id)}`, | |
| { | |
| method: 'POST', | |
| }, | |
| ); | |
| assert( | |
| this.response.ok, | |
| `Repost request failed: ${this.response.status} ${this.response.statusText}`, | |
| ); | |
| }); |
| const initialResponse = await fetchActivityPub( | ||
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', | ||
| { | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }, | ||
| ); | ||
| const initialResponseJson = await initialResponse.json(); | ||
| const firstPageReponse = await fetchActivityPub(initialResponseJson.first, { | ||
| headers: { | ||
| Accept: 'application/ld+json', | ||
| }, | ||
| }); | ||
| const outbox = await firstPageReponse.json(); | ||
|
|
||
| const found = (outbox.orderedItems || []).find((item) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle HTTP errors and fix typo
Same concerns as earlier helpers:
firstPageReponsetypo.- Missing
response.okchecks.
- const firstPageReponse = await fetchActivityPub(initialResponseJson.first, {
+ const firstPageResponse = await fetchActivityPub(initialResponseJson.first, {
headers: { Accept: 'application/ld+json' },
});
- const outbox = await firstPageReponse.json();
+ if (!firstPageResponse.ok) {
+ throw new Error(
+ `Outbox page fetch failed: ${firstPageResponse.status}`,
+ );
+ }
+ const outbox = await firstPageResponse.json();Committable suggestion skipped: line range outside the PR's diff.
no refs
Refactored cucumber steps into separate files, grouped by either feature or functionality (for generic steps).
stepdefs.jshas been renamed toindex.jsand contains the hooks to execute as well as any global setup