feat: more tests, tiny refactor #69
Conversation
…dby server to separate file, add content callback handler so we can work with extracted content in tests
There was a problem hiding this comment.
Pull Request Overview
This PR expands test coverage for the crawler components, refactors the standby server setup, and resolves several type annotation and linter issues.
- Updated Vitest configuration to properly include/exclude test files
- Added tests for both Playwright and Cheerio-based crawlers with a dedicated test server
- Refactored server, request handler, and main entrypoint to improve maintainability
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adjusted test include/exclude configuration for improved test file matching |
| tests/* | Introduced new tests for standby, Playwright, and Cheerio crawlers |
| tests/helpers/server.ts | Moved the test server logic to a dedicated file |
| src/utils.ts, src/types.ts | Updated imports to use proper type annotations and added a new result callback interface |
| src/server.ts | Refactored server setup with SSE support; improved error message in catch-all route |
| src/search.ts, src/responses.ts | Updated type imports for consistency |
| src/request-handler.ts | Enhanced result handling by injecting a callback to push results to the dataset |
| src/main.ts | Replaced inline express server logic with the centralized createServer function and improved port handling |
| src/input.ts, src/crawlers.ts | Minor refactoring of default parameter values and type annotations |
| .github/workflows/checks.yml | Added CI workflow to run tests, linting, and build steps |
Files not reviewed (1)
- tests/helpers/html/basic.html: Language not supported
Comments suppressed due to low confidence (1)
src/main.ts:24
- Since app.listen expects a number, consider converting 'process.env.ACTOR_STANDBY_PORT' to a number (e.g. using Number() or parseInt) to ensure the port is properly typed.
const port = Actor.isAtHome() ? process.env.ACTOR_STANDBY_PORT as string : 3000;
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Thank you for cleaning up the code and adding more tests. And since you're already cleaning up the code why not upgrade eslint to version 9 with the new configuration (https://www.notion.so/apify/New-linting-and-formatting-configuration-across-Apify-0b62890892cd4b71b4fe2dee9291b4af?pvs=4)?
Also I have few comments about the added complexity due to the new tests. I think there might be simpler ways to achieve the desired results.
Thank you for review and suggestion 👍 Changed eslint to 9 |
| @@ -26,13 +26,40 @@ export function getCrawlerKey(crawlerOptions: CheerioCrawlerOptions | Playwright | |||
| return JSON.stringify(crawlerOptions); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I don't get it, why are you moving this function on top? The order was logical, first you create crawlers, then you add requests.
You left addSearchRequest at the end, why?
There was a problem hiding this comment.
because of the linter - it said we are using XYZ before was defined
| @@ -65,6 +65,17 @@ export interface TimeMeasure { | |||
| timeDeltaPrevMs: number; | |||
| } | |||
|
|
|||
| export interface ContentScraperSettings { | |||
There was a problem hiding this comment.
Why are you moving ContentScraperSettings.
If you need to reorder it then I would put together Input and Output, it is an obvious choice.
There was a problem hiding this comment.
same here - linter said we are using interface before it was defined
Sure, here is the list of runs 👍 Normal cheerio: https://console.apify.com/view/runs/CJwySHsK5FERBmClg |
This PR introduces basic crawler tests, standby tests, fixes linter issues, moves standby server to separate file, and adds content callback handler so we can work with extracted content in tests