Added factory frame to E2E package #24614
Conversation
WalkthroughAdds a new TypeScript E2E data factory under e2e/data-factory: an abstract generic Factory, a PersistenceAdapter interface and a KnexPersistenceAdapter implementation, a PostFactory and Post type, and utilities (generateId, generateUuid, generateSlug). Includes README, .env.example, tests for PostFactory, Playwright and tsconfig updates to include the data-factory, an npm script "test:factory", new devDependencies (@faker-js/faker, knex, mysql2), and a CI change switching the DB client to mysql2. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (4)
e2e/data-factory/.env.example (1)
1-6: Fix minor formatting issues flagged by static analysis.The database configuration is appropriate for the data factory system. However, consider addressing the formatting issues identified by dotenv-linter:
# Ghost Database Configuration GHOST_DB_HOST=localhost -GHOST_DB_PORT=3306 -GHOST_DB_USER=root -GHOST_DB_PASSWORD=root GHOST_DB_NAME=ghost +GHOST_DB_PASSWORD=root +GHOST_DB_PORT=3306 +GHOST_DB_USER=root +e2e/data-factory/README.md (1)
7-7: Add language specification to fenced code block.The project structure diagram should specify a language identifier to improve rendering and accessibility.
Apply this diff to fix the issue:
-``` +```texte2e/data-factory/factories/factory.ts (1)
22-28: Consider improving type safety of the adapter.insert() return value.The type assertion on line 27 assumes that
adapter.insert()returns the exact same type structure asTResult. This could be unsafe if the database returns additional fields (like timestamps) or transforms the data.Consider one of these approaches:
- return this.adapter.insert(this.entityType, data) as Promise<TResult>; + return this.adapter.insert(this.entityType, data) as Promise<TResult>;Or update the
PersistenceAdapterinterface to be more type-safe:interface PersistenceAdapter { insert<T>(entityType: string, data: T): Promise<T>; }e2e/data-factory/persistence/adapters/knex.ts (1)
12-29: Improve robustness of ID extraction and consider edge cases.The MySQL workaround is well-documented, but the ID extraction could be more robust:
- const id = (data as {id?: string}).id; - if (!id) { - throw new Error('Cannot insert without an id field'); - } + const dataWithId = data as Record<string, unknown>; + const id = dataWithId.id; + if (!id || typeof id !== 'string') { + throw new Error('Cannot insert without a valid string id field'); + }Also consider: What if the entity is deleted between insert and findById? This could cause the method to throw even though the insert succeeded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e/data-factory/.env.example(1 hunks)e2e/data-factory/README.md(1 hunks)e2e/data-factory/factories/factory.ts(1 hunks)e2e/data-factory/factories/posts/post-factory.ts(1 hunks)e2e/data-factory/index.ts(1 hunks)e2e/data-factory/persistence/adapter.ts(1 hunks)e2e/data-factory/persistence/adapters/knex.ts(1 hunks)e2e/data-factory/post-factory.test.ts(1 hunks)e2e/data-factory/utils.ts(1 hunks)e2e/package.json(1 hunks)e2e/playwright.config.mjs(1 hunks)e2e/tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: niranjan-uma-shankar
PR: TryGhost/Ghost#24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
📚 Learning: set playwright_reporter=list environment variable before running playwright e2e tests as an ai agent...
Learnt from: CR
PR: TryGhost/Ghost#0
File: apps/comments-ui/.cursor/rules/playwright-e2e.mdc:0-0
Timestamp: 2025-07-19T21:18:10.726Z
Learning: Set PLAYWRIGHT_REPORTER=list environment variable before running Playwright e2e tests as an AI agent
Applied to files:
e2e/package.jsone2e/playwright.config.mjs
📚 Learning: in ghost, when node_env is set to testing-mysql, ghost uses config.testing-mysql.json configuration ...
Learnt from: cmraible
PR: TryGhost/Ghost#23941
File: .github/workflows/ci.yml:911-914
Timestamp: 2025-06-19T22:57:05.880Z
Learning: In Ghost, when NODE_ENV is set to testing-mysql, Ghost uses config.testing-mysql.json configuration which sets the server port to 2369 instead of the default 2368. This also applies to other testing environments like testing and testing-browser.
Applied to files:
e2e/data-factory/.env.example
📚 Learning: in the ghost repository, the path to defaults.json is correctly structured as 'ghost/core/core/share...
Learnt from: allouis
PR: TryGhost/Ghost#22127
File: .github/scripts/release-apps.js:72-74
Timestamp: 2025-02-06T10:30:53.440Z
Learning: In the Ghost repository, the path to defaults.json is correctly structured as 'ghost/core/core/shared/config/defaults.json' with an intentional double 'core' in the path.
Applied to files:
e2e/data-factory/.env.example
📚 Learning: the ghost docker compose setup has two independent profiles: `ghost` profile (v0, runs all apps in a...
Learnt from: cmraible
PR: TryGhost/Ghost#23546
File: compose.yml:58-59
Timestamp: 2025-05-27T18:08:00.458Z
Learning: The Ghost Docker Compose setup has two independent profiles: `ghost` profile (v0, runs all apps in a single container) and `split` profile (work in progress, runs Ghost server, admin, and frontend apps in separate services). The `split` profile will eventually replace `ghost` as the default.
Applied to files:
e2e/data-factory/.env.example
📚 Learning: ghost does not support postgresql and has no plans to support it. database migration utilities only ...
Learnt from: kevinansfield
PR: TryGhost/Ghost#24533
File: ghost/core/core/server/data/migrations/utils/schema.js:177-210
Timestamp: 2025-07-31T10:47:04.318Z
Learning: Ghost does not support PostgreSQL and has no plans to support it. Database migration utilities only need to handle SQLite and MySQL.
Applied to files:
e2e/data-factory/.env.example
📚 Learning: services that are dependencies for both ghost docker compose profiles (`ghost` and `split`) need to ...
Learnt from: cmraible
PR: TryGhost/Ghost#23546
File: compose.yml:58-59
Timestamp: 2025-05-27T18:08:00.458Z
Learning: Services that are dependencies for both Ghost Docker Compose profiles (`ghost` and `split`) need to include both profiles in their `profiles` configuration to ensure they start correctly under either profile.
Applied to files:
e2e/data-factory/.env.example
📚 Learning: in ghost admin-x-settings, webhook url validation uses {require_tld: false} to allow localhost urls ...
Learnt from: troyciesco
PR: TryGhost/Ghost#24291
File: apps/admin-x-settings/package.json:45-45
Timestamp: 2025-07-09T18:06:09.856Z
Learning: In Ghost admin-x-settings, webhook URL validation uses {require_tld: false} to allow localhost URLs for local integration testing, while social URL validation uses the default {require_tld: true} since social URLs should be public-facing.
Applied to files:
e2e/data-factory/.env.example
📚 Learning: in ghost project, app.js files under core/server/web are intentionally excluded from unit test cover...
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
Applied to files:
e2e/tsconfig.jsone2e/data-factory/README.md
📚 Learning: in ghost development, prs may depend on unpublished changes from sdk packages. when this occurs, typ...
Learnt from: niranjan-uma-shankar
PR: TryGhost/Ghost#24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Applied to files:
e2e/data-factory/README.md
📚 Learning: the pending activity utilities in the ghost activitypub module are covered by tests in the file `app...
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
e2e/data-factory/README.mde2e/data-factory/post-factory.test.tse2e/data-factory/factories/posts/post-factory.ts
📚 Learning: the pending activity utilities in ghost's activitypub module are thoroughly tested in `apps/admin-x-...
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
e2e/data-factory/README.mde2e/data-factory/post-factory.test.tse2e/data-factory/utils.tse2e/data-factory/factories/posts/post-factory.ts
📚 Learning: in the ghost test framework, when testing csv exports via the admin api, the response `body` field i...
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Applied to files:
e2e/data-factory/README.mde2e/data-factory/post-factory.test.ts
🧬 Code Graph Analysis (1)
e2e/data-factory/post-factory.test.ts (1)
e2e/data-factory/persistence/adapters/knex.ts (1)
KnexPersistenceAdapter(7-77)
🪛 dotenv-linter (3.3.0)
e2e/data-factory/.env.example
[warning] 5-5: [UnorderedKey] The GHOST_DB_PASSWORD key should go before the GHOST_DB_PORT key
[warning] 6-6: [EndingBlankLine] No blank line at the end of the file
[warning] 6-6: [UnorderedKey] The GHOST_DB_NAME key should go before the GHOST_DB_PASSWORD key
🪛 markdownlint-cli2 (0.17.2)
e2e/data-factory/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (23)
e2e/playwright.config.mjs (1)
15-16: LGTM! Configuration properly supports the new data factory system.The changes correctly expand test discovery to include the new
data-factorydirectory while maintaining the existing test structure. The explicittestMatchpatterns ensure both existing tests and new factory tests are discovered and executed.e2e/data-factory/persistence/adapter.ts (1)
4-11: Excellent interface design for persistence abstraction.The
PersistenceAdapterinterface provides a clean, type-safe abstraction for CRUD operations with:
- Generic type parameters for type safety
- Appropriate async signatures with Promise return types
- Standard CRUD method coverage (insert, update, delete, deleteMany, findById, findMany)
- Flexible
entityTypeparameter enabling multi-entity support- Proper use of
Partial<T>for update operationse2e/package.json (1)
15-15: Good addition for focused factory testing.The new
test:factoryscript provides a convenient way to run only the data factory tests, supporting focused development and testing of the new factory system.e2e/tsconfig.json (1)
108-109: Proper TypeScript configuration for the data factory system.The configuration changes correctly:
- Include the new
data-factory/**/*directory for compilation- Exclude test files from the build output (appropriate since they don't need compilation)
- Follow standard practices by excluding
node_modulesThis ensures the new factory system is properly compiled while keeping the build output clean.
e2e/data-factory/utils.ts (3)
6-10: LGTM! Proper MongoDB-style ObjectId generation.The implementation correctly combines a timestamp with random bytes to create unique identifiers that follow MongoDB ObjectId format.
15-28: LGTM! Correct UUID v4 implementation.The implementation properly sets the version bits (0x40) and variant bits (0x80) according to RFC 4122, and formats the UUID string correctly with hyphens.
33-41: LGTM! Robust slug generation logic.The implementation handles all common slug generation requirements: lowercasing, removing unwanted characters, replacing spaces with hyphens, collapsing multiple hyphens, and trimming edge hyphens.
e2e/data-factory/index.ts (1)
1-11: LGTM! Clean module organization and exports.The index file provides a well-structured entry point with clear categorization of exports. The separation of factories, adapters, and utilities makes it easy to understand the module's public API.
e2e/data-factory/README.md (1)
1-120: Excellent comprehensive documentation.The README provides clear setup instructions, usage examples, development guidelines, and project structure overview. This will significantly help developers understand and use the data factory system.
e2e/data-factory/post-factory.test.ts (3)
6-15: LGTM! Proper database configuration with environment variable support.The configuration correctly uses Ghost's development database defaults while allowing customization through environment variables.
22-46: LGTM! Proper test setup and cleanup.The test lifecycle management is well-implemented with database connection verification, proper adapter initialization, and thorough cleanup of test data.
48-95: LGTM! Comprehensive test coverage.The test cases cover all essential functionality: in-memory building, database persistence, unique data generation, and published_at logic. The assertions verify both the factory behavior and database integration.
e2e/data-factory/factories/posts/post-factory.ts (4)
5-32: LGTM! Comprehensive Post interface.The interface correctly defines all necessary fields for Ghost posts with appropriate types, including proper union types for status and nullable fields.
37-40: LGTM! Proper factory class structure.The factory correctly extends the base Factory class with appropriate generics and sets the required name and entityType properties.
41-84: LGTM! Realistic test data generation with proper defaults.The build method creates comprehensive defaults using faker for realistic data, proper mobiledoc structure for Ghost content, and appropriate default values for all post fields.
86-95: LGTM! Correct published_at business logic.The implementation properly handles the Ghost business rule that published posts should have a published_at timestamp, while draft posts should not. The logic correctly sets the timestamp when status is 'published' but no explicit published_at is provided.
e2e/data-factory/factories/factory.ts (3)
1-8: LGTM! Well-structured generic factory pattern.The TypeScript generics are properly constrained and the abstract base class design follows established patterns. The protected adapter property allows subclasses access while maintaining encapsulation.
9-11: LGTM! Clean adapter assignment.The setter method is appropriately simple and follows the adapter pattern correctly.
13-17: LGTM! Clear contract for subclasses.The abstract method is well-documented and uses appropriate TypeScript patterns with
Partial<TOptions>for optional parameters.e2e/data-factory/persistence/adapters/knex.ts (4)
1-10: LGTM! Clean dependency injection pattern.The class structure follows good practices with proper dependency injection and encapsulation of the Knex instance.
31-42: LGTM! Consistent pattern with insert method.The update operation follows a consistent pattern of operation-then-fetch, with appropriate error handling.
44-58: LGTM! Well-implemented delete operations.The empty array guard in
deleteManyis good defensive programming, and the batch delete usingwhereInis efficient.
60-76: LGTM! Flexible find operations with appropriate return types.The find methods are well-implemented. The
findManyquery parameter is very flexible (Record<string, unknown>), which provides good usability for testing scenarios while relying on Knex for SQL injection protection.
| const content = faker.lorem.paragraphs(3); | ||
|
|
||
| // Generate mobiledoc format | ||
| const mobiledoc = { |
There was a problem hiding this comment.
Arguably this should build Lexical. Not sure we care until we work on editor- or content-related tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/data-factory/tests/post-factory.test.ts(1 hunks)e2e/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/package.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: niranjan-uma-shankar
PR: TryGhost/Ghost#24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
📚 Learning: the pending activity utilities in the ghost activitypub module are covered by tests in the file `app...
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
e2e/data-factory/tests/post-factory.test.ts
📚 Learning: the pending activity utilities in ghost's activitypub module are thoroughly tested in `apps/admin-x-...
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Applied to files:
e2e/data-factory/tests/post-factory.test.ts
📚 Learning: in the ghost test framework, when testing csv exports via the admin api, the response `body` field i...
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Applied to files:
e2e/data-factory/tests/post-factory.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (3)
e2e/data-factory/tests/post-factory.test.ts (3)
1-15: LGTM! Clean imports and flexible database configuration.The database configuration properly uses environment variables with sensible defaults, which aligns with Ghost's standard practices and enables easy testing across different environments.
48-77: Well-structured test cases with comprehensive assertions.Both tests follow good testing practices with clear AAA structure and thorough validation of both in-memory building and database persistence. The second test properly verifies both the returned object and database state.
Note: Consider tracking the created post ID for proper cleanup (relates to the cleanup strategy comment above).
79-94: Excellent validation of factory core functionality.These tests effectively validate critical factory behaviors:
- Uniqueness across multiple builds (essential for test isolation)
- Status-based published_at logic (important business rule validation)
The test assertions are well-chosen and comprehensive.
| test.afterEach(async () => { | ||
| // Clean up test data | ||
| if (db) { | ||
| await db('posts').where('title', 'like', '%Test%').del(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test data cleanup strategy for reliability.
The current cleanup uses a broad pattern (title LIKE '%Test%') which is brittle and could miss test data or accidentally delete legitimate data. Consider these alternatives:
- Track created post IDs and delete by ID
- Use a more specific test marker (e.g., a test-specific prefix)
- Use transaction rollback for isolation
+ let createdPostIds: string[] = [];
+
test.afterEach(async () => {
// Clean up test data
if (db) {
- await db('posts').where('title', 'like', '%Test%').del();
+ if (createdPostIds.length > 0) {
+ await db('posts').whereIn('id', createdPostIds).del();
+ createdPostIds = [];
+ }
}
});Then track IDs in tests that persist data:
const post = await postFactory.create({
title: 'Test Persisted Post',
status: 'published'
});
+ createdPostIds.push(post.id);📝 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.
| test.afterEach(async () => { | |
| // Clean up test data | |
| if (db) { | |
| await db('posts').where('title', 'like', '%Test%').del(); | |
| } | |
| }); | |
| // Track created posts so we can clean up exactly what we create | |
| let createdPostIds: string[] = []; | |
| test.afterEach(async () => { | |
| // Clean up test data | |
| if (db) { | |
| if (createdPostIds.length > 0) { | |
| await db('posts') | |
| .whereIn('id', createdPostIds) | |
| .del(); | |
| createdPostIds = []; | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
In e2e/data-factory/tests/post-factory.test.ts around lines 41 to 46, the test
data cleanup uses a broad title pattern which risks deleting unintended data or
missing some test entries. To fix this, modify the tests to track the IDs of
created posts and then delete specifically by those IDs in the afterEach
cleanup. Alternatively, add a unique test-specific prefix to post titles and
filter cleanup by that prefix, or wrap each test in a transaction and roll it
back after completion to isolate test data.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24614 +/- ##
=======================================
Coverage 71.47% 71.47%
=======================================
Files 1530 1530
Lines 114912 114912
Branches 13847 13847
=======================================
+ Hits 82131 82133 +2
+ Misses 31796 31777 -19
- Partials 985 1002 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This looks really good to me! I think this is a really solid foundation to start building on, and I don't have any notes on the factory code itself.
My only note: I think we should use the post-factory in a real test, that:
- Creates a published post, using the factory
- Visits the Ghost frontend and asserts the post exists
Two main reasons for this:
- I think having tests for our tests is confusing — I think we should delete the
e2e/data-factory/testsdirectory - I want to make sure we don't have any kind of split brain problem — I'm a little confused about why we needed to change the database client, where we didn't before — just want to make sure the factories are using the same database as Ghost in CI, basically.
Other than that, good to merge from my POV!
|
@cmraible It seems we need some more available within Ghost to be able to do this with a browser test. We see a 404 when trying to load it on the frontend. I believe it's the same issue we see when trying to load the post - we don't have an author. I would rather ignore doing that here, because we would have to introduce another relation/factory. Alternatively, we could log in to Admin and view the Posts List, where it does show, but then we have setup/auth to deal with, which feels like another distraction. On the whole I agree w/ the suggestion, but for this set, I'm happy w/ simply testing the factory works w/ itself. |
|
Okay I had a little play with adding a test locally, and found a potential concern: visiting the post is 404'ing because of the way Ghost caches routes — it calculates the routes for each post on boot, then it updates the routes based on internal events. So when the Ghost server is running, and we insert a post into the DB, the routes don't exist. I'm wondering if it would make more sense to use the API rather than using the DB directly. I was ambivalent on DB vs API before, but this feels like a good reason to use the API (where possible). |
| import type {PersistenceAdapter} from '../persistence/adapter'; | ||
|
|
||
| export abstract class Factory<TOptions extends Record<string, unknown> = Record<string, unknown>, TResult = TOptions> { | ||
| abstract name: string; |
There was a problem hiding this comment.
I think we should make identification automatic, like here , with return this.constructor.name;.
It's one less thing to think about when creating new factories. The less manual work we need to do, the better.
|
|
||
| export abstract class Factory<TOptions extends Record<string, unknown> = Record<string, unknown>, TResult = TOptions> { | ||
| abstract name: string; | ||
| abstract entityType: string; |
There was a problem hiding this comment.
nit nit: entityType does work and it's correct, I am wondering though.., since entityType is related to adapter, can we use a more descriptive name to describe that, maybe something like adapterTarget ?
Since we need a comment in PostFactory to explain it, thats a sign we should update it, if we can. (if we have to have a comment, we should add it on parrent class I think, so it's easy to find and read on each new factory we create)
maybe it could be even in that class something like:
persiscence = {
adapter?: PersistenceAdapter,
entity: string
};
@cmraible the more high level we can make it(as long as it's not more work for us) the better , +1 for API as long as it's not going to be much slower, still we can support our parallelization plan and it's not more tedious to do cleanups |
I think as long as we're not inserting tons of data (i.e. hundreds/thousands of posts), the API won't be a major performance drawback — although I agree this is the biggest tradeoff of using the API vs the DB directly. On the other hand:
To be clear, I expect there will be cases where we need to directly insert data into the DB, so there's no wasted effort here — but I think there's a lot to be gained from using the API by default, and falling back to direct DB when necessary. |
agreed 100% @cmraible |
|
I'm absolutely fine with using the API where we can (and posts is a place where we can). However, it does make it more difficult to provide a slim proof-of-concept like we have here, and we would still need the KnexAdapter for some use cases. Primarily, we need to know the Admin API keys in order to use the API and coordinate that piece. I'd argue we don't need to block this based on the API use. If wanted, we can pare it down to a simpler use case (just demonstrate creating a post works), and we can follow up with a separate PR to implement the API handling. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e/data-factory/factories/factory.ts (3)
9-11: Constructor-based adapter injection looks good and addresses prior feedback.Clean, simple DI; matches previous suggestions to streamline factory construction.
3-5: Auto-derivenameto reduce boilerplate; allow override where needed.Deriving
nameavoids manual repetition across factories. Subclasses can still override if a different identifier is desired.-export abstract class Factory<TOptions extends Record<string, unknown> = Record<string, unknown>, TResult = TOptions> { - abstract name: string; - abstract entityType: string; +export abstract class Factory<TOptions extends Record<string, unknown> = Record<string, unknown>, TResult = TOptions> { + // Default identification; subclasses can override if they need a custom label + get name(): string { + return this.constructor.name; + } + abstract entityType: string;If you prefer a normalized name (e.g., "post" instead of "PostFactory"), override in the subclass:
// In PostFactory override get name() { return 'post'; }
5-5: Clarify whatentityTyperepresents (adapter target).A short docstring helps future factory authors understand it maps to the adapter’s resource (table/collection/API path).
- abstract entityType: string; + /** Target resource known to the persistence adapter (e.g., table, collection, API resource) */ + abstract entityType: string;
🧹 Nitpick comments (7)
e2e/data-factory/README.md (4)
7-24: Add a language to the fenced code block (markdownlint MD040).Specify a language to satisfy MD040 and improve rendering. "text" fits the directory tree.
-``` +```text e2e/data-factory/ # Source files (TypeScript) - committed to git ├── factories/ │ ├── factory.ts # Base factory class │ └── posts/ │ └── post-factory.ts # Post factory implementation ├── persistence/ │ ├── adapter.ts # Persistence interface │ └── adapters/ │ └── knex.ts # Knex SQL adapter ├── index.ts # Main exports ├── utils.ts # Utility functions └── test/ └── post-factory.test.js # Test file (JavaScript) e2e/build/data-factory/ # Compiled files (JavaScript) - gitignored └── ... (generated by TypeScript compiler) -``` +```
51-58: Make the usage example copy-pasteable by including the Knex import.This avoids confusion when readers try the snippet as-is.
// Import from the built files const {PostFactory, KnexPersistenceAdapter} = require('../build/data-factory'); // Set up +const knex = require('knex'); const db = knex(dbConfig); const adapter = new KnexPersistenceAdapter(db); const postFactory = new PostFactory(adapter);
91-95: Tighten the wording in “Build Process” bullets (minor grammar).Clarifies plurality and improves readability.
-- **TypeScript source files** (`*.ts`) are in `e2e/data-factory/` -- **Compiled JavaScript** (`*.js`) goes to `e2e/build/data-factory/` -- The `e2e/build/` directory is gitignored -- Always rebuild after making changes: `cd e2e && yarn build` +- **TypeScript source files** (`*.ts`) are in `e2e/data-factory/` +- **Compiled JavaScript files** (`*.js`) are output to `e2e/build/data-factory/` +- The `e2e/build/` directory is ignored by Git +- Always rebuild after making changes: `cd e2e && yarn build`
115-119: Document the caveat of direct DB inserts vs API usage for end-to-end flows.Given route-caching and other app-layer behaviors, it’s helpful to set expectations and steer users toward an API adapter when available, with Knex as a fallback.
## Notes - The test file (`test/post-factory.test.js`) is JavaScript because it's a standalone script - All factory code is TypeScript and must be compiled before use - The compiled files in `e2e/build/` should never be committed to git + - Direct DB inserts via the Knex adapter bypass some Ghost app-layer behaviors (e.g., route caching). + For end-to-end flows that interact with the running app, prefer an API-based adapter when available + and use the Knex adapter as a fallback for lower-level fixtures.e2e/data-factory/factories/factory.ts (3)
7-11: Minor: Use a parameter property and mark adapter readonly.This trims a few lines and signals the adapter isn’t expected to change at runtime.
- protected adapter?: PersistenceAdapter; - - constructor(adapter?: PersistenceAdapter) { - this.adapter = adapter; - } + constructor(protected readonly adapter?: PersistenceAdapter) {}
22-28: Remove redundantawaiton return.No local try/catch; returning the promise directly is equivalent and slightly cleaner.
- return await this.adapter.insert(this.entityType, data) as Promise<TResult>; + return this.adapter.insert(this.entityType, data) as Promise<TResult>;
3-3: Avoid Partial-of-Partial by separating “build options” from “result” generics.When subclasses pass
Partial<T>asTOptions,build(options?: Partial<TOptions>)becomesPartial<Partial<T>>. UsingTBuildclarifies intent and avoids nested Partial.-export abstract class Factory<TOptions extends Record<string, unknown> = Record<string, unknown>, TResult = TOptions> { +export abstract class Factory<TBuild extends Record<string, unknown> = Record<string, unknown>, TResult = TBuild> { @@ - abstract build(options?: Partial<TOptions>): TResult; + abstract build(options?: Partial<TBuild>): TResult;Follow-up: Update subclasses accordingly, e.g.,
class PostFactory extends Factory<Post, Post> { ... }.Also applies to: 17-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
e2e/data-factory/README.md(1 hunks)e2e/data-factory/factories/factory.ts(1 hunks)e2e/data-factory/factories/posts/post-factory.ts(1 hunks)e2e/data-factory/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/data-factory/utils.ts
- e2e/data-factory/factories/posts/post-factory.ts
🧰 Additional context used
🪛 LanguageTool
e2e/data-factory/README.md
[grammar] ~91-~91: There might be a mistake here.
Context: ...peScript source files** (*.ts) are in e2e/data-factory/ - Compiled JavaScript (*.js) goes to `...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...Compiled JavaScript* (*.js) goes to e2e/build/data-factory/ - The e2e/build/ directory is gitignored...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/data-factory/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
e2e/data-factory/README.md
Outdated
| └── test/ | ||
| └── post-factory.test.js # Test file (JavaScript) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the test directory naming (“test/” vs “tests/”) is consistent with Playwright config.
The README shows test/ in the structure and references test/post-factory.test.js. Double-check this matches the repo paths and Playwright’s testDir to avoid confusion.
Run this from the repo root to confirm the path and config:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for the post-factory test file:"
fd -a -H -t f 'post-factory.test.*' | sed 's/^/ - /'
echo ""
echo "Checking Playwright config(s) for testDir:"
fd -a -H -t f 'playwright.config.*' e2e | while read -r cfg; do
echo "File: $cfg"
rg -nP -C2 '\btestDir\b' "$cfg" || true
echo ""
doneLength of output: 753
Align README test directory with the actual tests/ folder and file extension
The e2e/data-factory README currently shows a test/ directory and .test.js file, but the repository and Playwright config use tests/ and a .test.ts file. Please update the snippet in e2e/data-factory/README.md (lines 19–21) accordingly:
- Replace
test/withtests/ - Update the file extension from
.jsto.ts
Suggested diff:
- └── test/
- └── post-factory.test.js # Test file (JavaScript)
+ └── tests/
+ └── post-factory.test.ts # Test file (TypeScript)This will keep the README in sync with:
- Actual path:
e2e/data-factory/tests/post-factory.test.ts - Playwright config’s
testMatch: ['tests/**/*.test.{js,ts}', ...]
📝 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.
| └── test/ | |
| └── post-factory.test.js # Test file (JavaScript) | |
| └── tests/ | |
| └── post-factory.test.ts # Test file (TypeScript) |
🤖 Prompt for AI Agents
In e2e/data-factory/README.md around lines 19 to 21, the README shows a `test/`
directory and `post-factory.test.js` which are out of sync with the repo; update
the snippet to use `tests/` and change the filename extension to `.ts` so it
reads `tests/post-factory.test.ts`, ensuring the README matches the actual path
and Playwright testMatch.
|
@ibalosh I've changed the suggestions you had (all good ones, thanks!). I simply resolved the convos instead of responding to each. Mind taking a pass and approving when you feel it's good? |
There was a problem hiding this comment.
I could add one nitpick (like I would love if we ditch should in tests :) ) , but it would not make sense to fiddle with that here.
Ready for merge in my opinion too, nice one @9larsons ! 👌
e2e/data-factory/README.md
Outdated
| │ └── knex.ts # Knex SQL adapter | ||
| ├── index.ts # Main exports | ||
| ├── utils.ts # Utility functions | ||
| └── test/ |
|
Those tests will be scrapped, they are just there for proof of concept. Fixed the readme and I'll merge 👍 . |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/data-factory/README.md (1)
19-21: Align test filename with actual repo (.ts, not .js).The project structure lists a
.jstest, but the repo usestests/post-factory.test.ts.-└── tests/ - └── post-factory.test.js # Test file (JavaScript) +└── tests/ + └── post-factory.test.ts # Test file (TypeScript)
🧹 Nitpick comments (4)
e2e/data-factory/README.md (4)
7-7: Add a language to the fenced code block.Markdownlint MD040: specify a language. Use
textfor directory trees.-``` +```text
50-70: Make the usage snippet executable: import knex and wrapawaitin an async context.Currently
knexanddbConfigare undefined andawaitis used at top level with CommonJSrequire.-```javascript +```javascript // Import from the built files -const {PostFactory, KnexPersistenceAdapter} = require('../build/data-factory'); +const {PostFactory, KnexPersistenceAdapter} = require('../../build/data-factory'); // Set up -const db = knex(dbConfig); +const knex = require('knex'); +const dbConfig = {/* configure or reuse Ghost's DB config */}; +const db = knex(dbConfig); const adapter = new KnexPersistenceAdapter(db); const postFactory = new PostFactory(adapter); // Build in memory (not persisted) const draftPost = postFactory.build({ title: 'My Draft', status: 'draft' }); // Create and persist -const publishedPost = await postFactory.create({ - title: 'My Published Post', - status: 'published' -}); +(async () => { + const publishedPost = await postFactory.create({ + title: 'My Published Post', + status: 'published' + }); +})();
115-119: Add a note about route-caching when using direct DB writes; prefer API adapter when routes matter.Direct inserts via Knex don’t trigger Ghost’s internal events (e.g., route cache updates). This can cause 404s in browser tests after creating posts. Add guidance to use an API-based adapter when you need router-visible entities; keep Knex for fast, internal-only setup.
## Notes -- The test file (`test/post-factory.test.js`) is JavaScript because it's a standalone script +- The test file (`tests/post-factory.test.ts`) is a Playwright test (TypeScript) - All factory code is TypeScript and must be compiled before use - The compiled files in `e2e/build/` should never be committed to git +- +- When using the KnexPersistenceAdapter (direct DB writes), Ghost’s route cache + is not updated. Browser tests that navigate to newly created entities may 404. + Prefer an API-based adapter when routes/derived state must be updated; keep + Knex as a fast fallback when end-to-end behaviors are not required.
91-95: Minor grammar polish in Build Process.Tighten phrasing and avoid “gitignored.”
-- **TypeScript source files** (`*.ts`) are in `e2e/data-factory/` -- **Compiled JavaScript** (`*.js`) goes to `e2e/build/data-factory/` -- The `e2e/build/` directory is gitignored +- **TypeScript source files** (`*.ts`) are in `e2e/data-factory/` +- **Compiled JavaScript** (`*.js`) is emitted to `e2e/build/data-factory/` +- The `e2e/build/` directory is ignored by Git
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
e2e/data-factory/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
e2e/data-factory/README.md
[grammar] ~91-~91: There might be a mistake here.
Context: ...peScript source files** (*.ts) are in e2e/data-factory/ - Compiled JavaScript (*.js) goes to `...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...Compiled JavaScript* (*.js) goes to e2e/build/data-factory/ - The e2e/build/ directory is gitignored...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
e2e/data-factory/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
|
|
||
| ```javascript | ||
| // Import from the built files | ||
| const {PostFactory, KnexPersistenceAdapter} = require('../build/data-factory'); |
There was a problem hiding this comment.
Fix incorrect import path to built artifacts from tests.
From e2e/data-factory/tests/, the correct relative path to e2e/build/data-factory is ../../build/data-factory, not ../build/data-factory.
-const {PostFactory, KnexPersistenceAdapter} = require('../build/data-factory');
+const {PostFactory, KnexPersistenceAdapter} = require('../../build/data-factory');📝 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 {PostFactory, KnexPersistenceAdapter} = require('../build/data-factory'); | |
| const {PostFactory, KnexPersistenceAdapter} = require('../../build/data-factory'); |
🤖 Prompt for AI Agents
In e2e/data-factory/README.md around line 52, the example import path is
incorrect for tests run from e2e/data-factory/tests; update the require path
from ../build/data-factory to the correct relative path ../../build/data-factory
so the tests resolve the built artifacts; edit that line in the README to use
the two-level-up path.
|
|
||
| ## Notes | ||
|
|
||
| - The test file (`test/post-factory.test.js`) is JavaScript because it's a standalone script |
There was a problem hiding this comment.
Fix path and extension in Notes; it’s TypeScript under tests/.
The note references test/ and .js, which is inconsistent with the repo.
-- The test file (`test/post-factory.test.js`) is JavaScript because it's a standalone script
+- The test file (`tests/post-factory.test.ts`) is a Playwright test (TypeScript)📝 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.
| - The test file (`test/post-factory.test.js`) is JavaScript because it's a standalone script | |
| - The test file (`tests/post-factory.test.ts`) is a Playwright test (TypeScript) |
🤖 Prompt for AI Agents
In e2e/data-factory/README.md around line 117, the Note incorrectly references
the test path and extension as `test/post-factory.test.js`; update the text to
reflect the actual file location and TypeScript extension (e.g.,
`tests/post-factory.test.ts`) so the README matches the repo structure and
language; replace the reference and any related wording accordingly.
ref https://linear.app/ghost/issue/PROD-2327/create-e2e-test-data-factory
This PR adds the basic building blocks for data factories to be used for the e2e package, and possibly extended to other areas. There's a few key design decisions:
Factorys should be scoped to a single model, and only be responsible for creationbuildandcreatespecify creating in memory and persisting an object.FactoryandPersistenceAdapterneed to be generalizable for all future data sources.The overarching goal of this is to enable easy test writing.
We will further refine this by adding additional factories and data sources in future PRs, and we may go down the road of wrapping
PostFactory,UserFactory, etc. in a Ghost handler/coordinator, which is straightforward change with this structure.