Refactored cucumber setup to allow for multiple step definition files #601
Conversation
no refs
Miscellaneous refactorings to allow us to move to using multiple step definition
files instead of one large file:
- Introduced `support` directory to organize support files
- Moved generic helper functions out of `stepdefs.js` and into their
respective support files
- Removed nearly all global state from `stepdefs.js`
- Support files keep track of there global state (i.e `db.js` keeping track
of database client instance)
- Added `block_steps.js` to organize block related step definitions
- Moved global static constants to environment variables
I opted to just use the built in `World` rather than a custom one - We could
maybe move to this at a later point and use it to keep track of global state etc
WalkthroughThis change introduces a modular refactor and enhancement of the Cucumber test suite for ActivityPub integration. Two new environment variables, The step definitions file ( Possibly related PRs
✨ 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: 4
🧹 Nitpick comments (6)
features/support/db.js (1)
24-40: Database reset function is comprehensive.The reset function properly handles foreign key constraints and truncates all relevant tables. This supports the PR objective of delegating global state management to dedicated support files.
Consider adding a
close()function to explicitly clean up database connections when tests complete:+export async function close() { + if (client) { + await client.destroy(); + client = null; + } +}features/support/utils.js (1)
1-3: Utility function for asynchronous testing.This utility function supports the PR objective of moving generic helper functions out of the main step definitions file.
Consider improving parameter naming and adding JSDoc for better clarity:
-export async function wait(n) { - return new Promise((resolve) => setTimeout(resolve, n)); +/** + * Returns a promise that resolves after the specified number of milliseconds. + * @param {number} milliseconds - The number of milliseconds to wait + * @returns {Promise<void>} + */ +export async function wait(milliseconds) { + return new Promise((resolve) => setTimeout(resolve, milliseconds));features/support/steps.js (2)
9-20: Be careful with the greedy regex pattern.The current pattern
(\w+)\((.+)\)uses a greedy matcher.+which could cause issues with nested parentheses. For example, parsing "Create(Note(Special))" would result in activity="Create" and object="Note(Special" instead of properly handling the nested structure.Consider using a non-greedy pattern or a more specific pattern if nested parentheses need to be supported:
- const [match, activity, object] = string.match(/(\w+)\((.+)\)/) || [null]; + const [match, activity, object] = string.match(/(\w+)\(([^)]+)\)/) || [null];
30-40: DRY opportunity: parseActorString duplicates logic from parseActivityString.Both functions share the same pattern matching and error handling logic, differing only in the property names returned.
Consider refactoring to avoid duplication:
+ /** + * Generic function to parse strings like `Word(Content)` + * + * @param {string} string + * @param {[string, string]} propertyNames - Names for the two extracted parts + * + * @returns {Object} + */ + function parseGenericString(string, [firstProp, secondProp]) { + const [match, first, second] = string.match(/(\w+)\(([^)]+)\)/) || [null]; + if (!match) { + const result = {}; + result[firstProp] = null; + result[secondProp] = null; + return result; + } + const result = {}; + result[firstProp] = first; + result[secondProp] = second; + return result; + } + + export function parseActivityString(string) { + return parseGenericString(string, ['activity', 'object']); + } + + export function parseActorString(string) { + return parseGenericString(string, ['type', 'name']); + }features/support/wiremock.js (1)
27-30: Consider adding error handling in the reset function.The
resetfunction calls methods on the WireMock instances but doesn't handle potential errors. In a test environment, errors here could lead to confusing test failures.Consider adding try/catch blocks:
export function reset() { - getExternalActivityPub().clearAllRequests(); - getGhostActivityPub().clearAllRequests(); + try { + getExternalActivityPub().clearAllRequests(); + getGhostActivityPub().clearAllRequests(); + } catch (error) { + console.error('Failed to reset WireMock instances:', error); + // Depending on your error handling strategy, you might want to rethrow + // or handle it differently + } }features/support/request.js (1)
41-64: Consider increasing default timeout and using non-recursive implementation.The current implementation has two potential issues:
- The default timeout (1000ms) might be too short for some environments, leading to flaky tests
- The recursive approach could potentially cause stack overflow with long timeouts and small step values
Here's an alternative non-recursive implementation with a longer default timeout:
export async function waitForRequest( method, path, matcher, step = 100, - milliseconds = 1000, + milliseconds = 5000, ) { const externalActivityPub = getExternalActivityPub(); - const calls = await externalActivityPub.getRequestsForAPI(method, path); - const found = calls.find(matcher); - - if (found) { - return found; - } - - if (milliseconds <= 0) { - return null; - } - - await wait(step); - - return waitForRequest(method, path, matcher, step, milliseconds - step); + const startTime = Date.now(); + const endTime = startTime + milliseconds; + + while (Date.now() < endTime) { + const calls = await externalActivityPub.getRequestsForAPI(method, path); + const found = calls.find(matcher); + + if (found) { + return found; + } + + await wait(step); + } + + return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
docker-compose.yml(1 hunks)features/step_definitions/block_steps.js(1 hunks)features/step_definitions/stepdefs.js(5 hunks)features/support/db.js(1 hunks)features/support/fixtures.js(1 hunks)features/support/request.js(1 hunks)features/support/steps.js(1 hunks)features/support/utils.js(1 hunks)features/support/wiremock.js(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
features/step_definitions/block_steps.js (2)
features/step_definitions/stepdefs.js (10)
actor(122-122)actor(608-608)actor(626-626)actor(657-657)actor(880-880)actor(1137-1137)actor(1156-1156)actor(1175-1175)actor(1195-1195)actor(1587-1587)features/support/request.js (1)
fetchActivityPub(12-39)
features/support/wiremock.js (4)
features/support/request.js (1)
externalActivityPub(48-48)features/support/fixtures.js (2)
externalActivityPub(198-198)externalActivityPub(292-292)features/step_definitions/stepdefs.js (1)
ghostActivityPub(47-47)features/support/db.js (1)
reset(24-40)
features/support/steps.js (1)
features/step_definitions/stepdefs.js (35)
parseActivityString(1064-1064)match(1047-1047)activity(378-378)activity(633-633)activity(664-664)activity(698-698)activity(814-814)activity(823-823)activity(882-882)activity(906-906)activity(1094-1094)activity(1102-1102)activity(1119-1119)activity(1271-1271)activity(1297-1297)activity(1379-1379)activity(1407-1407)object(188-189)object(377-377)object(440-440)object(457-457)object(484-484)object(503-503)object(531-531)object(562-562)object(609-609)object(627-631)object(658-662)object(1103-1103)object(1197-1198)object(1308-1308)object(1326-1326)parseActorString(273-273)type(289-289)name(290-290)
features/support/db.js (1)
features/support/wiremock.js (1)
reset(27-30)
features/support/request.js (2)
features/support/wiremock.js (2)
externalActivityPub(4-4)getExternalActivityPub(9-17)features/support/utils.js (1)
wait(1-3)
features/step_definitions/stepdefs.js (3)
features/support/request.js (2)
__filename(9-9)__dirname(10-10)features/support/wiremock.js (2)
ghostActivityPub(7-7)getGhostActivityPub(19-25)features/support/db.js (1)
getClient(6-22)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (9)
docker-compose.yml (1)
177-178: Environment variables for ActivityPub endpoints have been properly configured.The addition of these environment variables aligns with the PR objective of moving global static constants to environment variables. The URLs correctly point to the Wiremock service aliases defined in the network configuration.
features/support/db.js (1)
1-22: Well-structured database client singleton implementation.The implementation follows the singleton pattern for database connection management, which ensures efficient resource usage. Configuration is properly sourced from environment variables.
package.json (1)
41-41:Details
❓ Verification inconclusive
Cucumber dependency has been updated.
The update from 10.x to 11.2.0 likely supports the refactoring changes to allow for multiple step definition files.
🌐 Web query:
What are the major changes between Cucumber.js 10.x and 11.x?💡 Result:
Major Changes Between Cucumber.js 10.x and 11.x
Cucumber.js 11.x introduced several important changes compared to the 10.x series, including breaking changes, removals, and technical improvements. Here’s a summary of the most significant updates:
Breaking Changes and Removals
Removed Deprecated APIs: Cucumber.js 11.0.0 removed several previously deprecated components:
parseGherkinMessageStreamPickleFilterRuntime
These removals may impact projects relying on these APIs and require code refactoring during the upgrade process[1].Dropped Support for Node.js 21: Cucumber.js 11.x no longer supports Node.js 21, so users must upgrade to a supported version (such as Node.js 22 or above)[1].
Internal Refactoring
- Major Refactor of Runtime: The internal runtime code was refactored to improve both serial and parallel execution modes. This change aims to enhance maintainability and performance but may affect custom tool integrations or advanced usages[1].
Formatter and Plugin Updates
JUnit Formatter Replacement: The JUnit formatter has been replaced with a messages-based package, changing how test outputs are formatted and reported[1].
Error and Status Reporting: Improvements to error handling and status reporting were made, such as correct reporting in the publish plugin and better handling of UNKNOWN test statuses[1].
Node.js Support
- Addition of Node.js 23 Support: Official support for Node.js 23 has been introduced, ensuring compatibility with the latest Node.js features[1].
Other Notable Fixes and Changes
- Parallel Execution: Parallel mode terminology was changed from “master/slave” to “coordinator/worker,” and related environment variables were updated in prior versions, setting the stage for future deprecations[2].
- Export Updates: Some exports were fixed or added, like the missing
setParallelCanAssignexport in 11.0.1[1].Summary Table
Change Details Deprecated API Removal parseGherkinMessageStream,PickleFilter,RuntimeremovedNode.js Support Node.js 21 dropped, Node.js 23 added Runtime Refactor Major internal refactor for serial and parallel modes JUnit Formatter Replaced with messages-based package Error/Status Reporting Improved error handling and UNKNOWN status rendering Parallel Execution Naming “Coordinator/worker” replace “master/slave” terminology For a full list of detailed changes and migration advice, refer to the official changelog and upgrading guide[1][2].
Citations:
- 1: https://github.com/cucumber/cucumber-js/blob/main/CHANGELOG.md
- 2: https://github.com/cucumber/cucumber-js/blob/main/UPGRADING.md
- 3: https://cucumber.io/docs/guides/upgrading
- 4: https://cucumber.io/docs/guides/10-minute-tutorial
- 5: https://webdriver.io/blog/2019/07/11/cucumberjs-v5-released/
- 6: https://www.youtube.com/watch?v=MGldZa6amD8
- 7: https://raw.githubusercontent.com/cucumber/cucumber-js/master/CHANGELOG.md
- 8: https://raw.githubusercontent.com/cucumber/cucumber-js/cc6c89e9ae35465b3fcbf777e7a3e555fea7ff61/CHANGELOG.md
- 9: https://community.smartbear.com/discussions/cucumberos/what-are-the-new-features-that-will-get-updated-cucumber-from-info-cukes-1-2-5-t/203371
- 10: https://cucumber.io/docs/guides/10-minute-tutorial/
Review Cucumber.js Upgrade & Verify Impact
Cucumber.js 11.2.0 introduces several breaking changes and removals compared to the 10.x series. Before approving this dependency bump, please verify that your refactoring for multiple step-definition files does not rely on any of the removed or changed APIs and that your test suite still passes.
Key changes in 11.x:
- Removed deprecated APIs:
•parseGherkinMessageStream
•PickleFilter
•Runtime- Node.js support updates:
• Dropped support for Node.js 21
• Added support for Node.js 23- Internal runtime refactor for serial/parallel execution
- JUnit formatter replaced by a messages-based package
- Improved error/status reporting (publish plugin, UNKNOWN statuses)
- Renamed parallel mode terminology to “coordinator/worker”
Next steps:
- Confirm your code doesn’t use any removed APIs.
- Run the full test suite (including parallel mode) to catch regressions.
- Review the official Changelog & Upgrading Guide for detailed migration advice:
https://github.com/cucumber/cucumber-js/blob/main/CHANGELOG.md
https://github.com/cucumber/cucumber-js/blob/main/UPGRADING.mdfeatures/step_definitions/block_steps.js (1)
1-14: Clear, concise implementation for the blocking step.This new step definition cleanly implements actor blocking functionality by retrieving the actor from the context and making a POST request to the appropriate endpoint. The code is well-structured and concise, making good use of the refactored support modules.
features/support/wiremock.js (1)
1-25: Good use of the singleton pattern for WireMock instances.The implementation properly ensures only one instance of each WireMock service exists using lazy initialization. This is an efficient approach that aligns with the PR's goal of better state management.
features/support/request.js (1)
12-39: Well-structured authentication implementation.The
fetchActivityPubfunction cleanly handles JWT token generation and authentication. This is a good extraction of functionality that was likely duplicated across multiple step definitions previously.One minor suggestion: Consider making the JWT payload configurable for test cases that might need different users or roles.
features/step_definitions/stepdefs.js (3)
20-33: Imports correctly decoupled – nice clean-upMoving DB, fixture, request and WireMock helpers into dedicated support modules dramatically reduces the size of this file and clarifies responsibilities. 👍
102-110: Hard-fail risk ifURL_GHOST_ACTIVITY_PUBis unset
new URL(process.env.URL_GHOST_ACTIVITY_PUB)will throw aTypeErrorwhen the env-var is missing or empty, causing all scenarios to fail before they start.-const [siteId] = await getClient()('sites').insert({ - host: new URL(process.env.URL_GHOST_ACTIVITY_PUB).host, +const urlGhost = process.env.URL_GHOST_ACTIVITY_PUB; +assert( + urlGhost, + 'URL_GHOST_ACTIVITY_PUB env variable must be defined for test bootstrap', +); +const [siteId] = await getClient()('sites').insert({ + host: new URL(urlGhost).host,
126-143: Possible DB column overflow for JWK blobs
exportJwk(keypair.privateKey)serialises the entire RSA/EC key. Depending on the schema,ap_public_keyandap_private_keyare often limited tovarchar(255)ortext. The JSON string here can exceed 1 KiB.Confirm column types are
text(or similar) or truncate the material.
| const user = { | ||
| '@context': [ | ||
| 'https://www.w3.org/ns/activitystreams', | ||
| 'https://w3id.org/security/data-integrity/v1', | ||
| ], | ||
| id: `http://fake-external-activitypub.test/user/${name}`, | ||
| url: `http://fake-external-activitypub.test/user/${name}`, | ||
| type, | ||
|
|
||
| handle: `@${name}@fake-external-activitypub.test`, | ||
|
|
||
| preferredUsername: name, | ||
| name, | ||
| summary: 'A test actor for testing', | ||
|
|
||
| inbox: `http://fake-external-activitypub.test/inbox/${name}`, | ||
| outbox: `http://fake-external-activitypub.test/inbox/${name}`, | ||
| followers: `http://fake-external-activitypub.test/followers/${name}`, | ||
| following: `http://fake-external-activitypub.test/following/${name}`, | ||
| liked: `http://fake-external-activitypub.test/liked/${name}`, | ||
|
|
There was a problem hiding this comment.
Outbox URL copy-paste error
For remote actors the outbox property is set to an inbox URL:
outbox: `http://fake-external-activitypub.test/inbox/${name}`,Should be:
-outbox: `http://fake-external-activitypub.test/inbox/${name}`,
+outbox: `http://fake-external-activitypub.test/outbox/${name}`,Otherwise client libraries fetching the actor’s outbox will hit the wrong route.
| if (remote === false) { | ||
| return { | ||
| '@context': [ | ||
| 'https://www.w3.org/ns/activitystreams', | ||
| 'https://w3id.org/security/data-integrity/v1', | ||
| ], | ||
| id: 'http://fake-ghost-activitypub.test/.ghost/activitypub/users/index', | ||
| url: 'http://fake-ghost-activitypub.test/.ghost/activitypub/users/index', | ||
| type, | ||
|
|
||
| handle: '@index@fake-ghost-activitypub.test', | ||
|
|
||
| preferredUsername: 'index', | ||
| name, | ||
| summary: 'A test actor for testing', | ||
|
|
||
| inbox: 'http://fake-ghost-activitypub.test/.ghost/activitypub/inbox/index', | ||
| outbox: 'http://fake-ghost-activitypub.test/.ghost/activitypub/outbox/index', | ||
| followers: | ||
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/followers/index', | ||
| following: | ||
| 'http://fake-ghost-activitypub.test/.ghost/activitypub/following/index', | ||
| liked: 'http://fake-ghost-activitypub.test/.ghost/activitypub/liked/index', | ||
|
|
||
| 'https://w3id.org/security#publicKey': { | ||
| id: 'http://fake-ghost-activitypub.test/.ghost/activitypub/users/index#main-key', | ||
| type: 'https://w3id.org/security#Key', | ||
| 'https://w3id.org/security#owner': { | ||
| id: 'http://fake-ghost-activitypub.test/.ghost/activitypub/users/index', | ||
| }, | ||
| 'https://w3id.org/security#publicKeyPem': | ||
| '-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAtSc3IqGjRaO3vcFdQ15D\nF90WVJC6tb2QwYBh9kQYVlQ1VhBiF6E4GK2okvyvukIL5PHLCgfQrfJmSiopk9Xo\n46Qri6rJbcPoWoZz/jWN0pfmU20hNuTQx6ebSoSkg6rHv1MKuy5LmDGLFC2ze3kU\nsY8u7X6TOBrifs/N+goLaH3+SkT2hZDKWJrmDyHzj043KLvXs/eiyu50M+ERoSlg\n70uO7QAXQFuLMILdy0UNJFM4xjlK6q4Jfbm4MC8QRG+i31AkmNvpY9JqCLqu0mGD\nBrdfJeN8PN+7DHW/Pzspf5RlJtlvBx1dS8Bxo2xteUyLGIaTZ9HZFhHc3IrmmKeW\naQIDAQAB\n-----END PUBLIC KEY-----\n', | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Local (“remote = false”) actor uses hard-coded host
To stay consistent with the newly introduced URL_GHOST_ACTIVITY_PUB, build URLs from it instead of the literal fake-ghost-activitypub.test.
-const base = 'http://fake-ghost-activitypub.test';
+const base = process.env.URL_GHOST_ACTIVITY_PUB;Update every derived path (users/index, inbox/index, …).
Committable suggestion skipped: line range outside the PR's diff.
| export async function createObject(type, actor, content) { | ||
| const object = generateObject(type, content); | ||
|
|
||
| if (!object) { | ||
| throw new Error(`Cannot create objects of type ${type}`); | ||
| } | ||
|
|
||
| object.attributedTo = actor.id; | ||
|
|
||
| const url = new URL(object.id); | ||
|
|
||
| getExternalActivityPub().register( | ||
| { | ||
| method: 'GET', | ||
| endpoint: url.pathname, | ||
| }, | ||
| { | ||
| status: 200, | ||
| body: object, | ||
| headers: { | ||
| 'Content-Type': 'application/activity+json', | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| return object; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
createObject still hard-codes the external host – undermines new env strategy
The PR introduces URL_EXTERNAL_ACTIVITY_PUB, but generateObject/createObject build URLs with the literal fake-external-activitypub.test. This diverges from the docker-compose settings and defeats easy re-configuration.
- id: `http://fake-external-activitypub.test/${type.toLowerCase()}/${uuid}`,
- url: `http://fake-external-activitypub.test/${type.toLowerCase()}/${uuid}`,
+ id: `${process.env.URL_EXTERNAL_ACTIVITY_PUB}/${type.toLowerCase()}/${uuid}`,
+ url: `${process.env.URL_EXTERNAL_ACTIVITY_PUB}/${type.toLowerCase()}/${uuid}`,Replicate this replacement for every hard-coded occurrence in the file.
Committable suggestion skipped: line range outside the PR's diff.
| export async function createActivity(type, object, actor) { | ||
| let activity; | ||
|
|
||
| if (type === 'Follow') { | ||
| activity = { | ||
| '@context': [ | ||
| 'https://www.w3.org/ns/activitystreams', | ||
| 'https://w3id.org/security/data-integrity/v1', | ||
| ], | ||
| type: 'Follow', | ||
| id: `${process.env.URL_EXTERNAL_ACTIVITY_PUB}/follow/${uuidv4()}`, | ||
| to: 'as:Public', | ||
| object: object, | ||
| actor: actor, | ||
| }; | ||
| } | ||
|
|
||
| if (type === 'Accept') { | ||
| activity = { | ||
| '@context': [ | ||
| 'https://www.w3.org/ns/activitystreams', | ||
| 'https://w3id.org/security/data-integrity/v1', | ||
| ], | ||
| type: 'Accept', | ||
| id: `${process.env.URL_EXTERNAL_ACTIVITY_PUB}/accept/${uuidv4()}`, | ||
| to: 'as:Public', | ||
| object: object, | ||
| actor: actor, | ||
| }; | ||
| } | ||
|
|
||
| if (type === 'Reject') { | ||
| activity = { | ||
| '@context': [ | ||
| 'https://www.w3.org/ns/activitystreams', | ||
| 'https://w3id.org/security/data-integrity/v1', |
There was a problem hiding this comment.
Missing guard: unknown type crashes tests
If an unsupported activity type is passed, activity remains undefined and the subsequent activity.id access throws. Add an explicit check:
@@
let activity;
...
}
+if (!activity) {
+ throw new Error(`Unsupported activity type "${type}"`);
+}
no refs
Miscellaneous refactorings to allow us to move to using multiple step definition files instead of one large file:
supportdirectory to organize support filesstepdefs.jsand into their respective support filesstepdefs.jsdb.jskeeping track of database client instance)block_steps.jsto organize block related step definitionsI opted to just use the built in
Worldrather than a custom one - We could maybe move to this at a later point and use it to keep track of global state etc