Fix server test coverage script#5843
Conversation
Summary of ChangesHello @yau-wd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the server's testing infrastructure by addressing compatibility issues with ESM-only modules in Jest and providing a dedicated command for generating test coverage reports. These changes streamline the testing process and improve the ability to monitor code coverage for the server package. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with the server test coverage script. It introduces a moduleNameMapper in the Jest configuration to handle ESM-only SDK imports and adds a test:coverage script to package.json. The changes are functional, but I've provided a couple of suggestions to improve maintainability and conciseness in both files.
yau-wd
requested review from
HenryHengZJ,
christopherholland-workday and
jocelynlin-wd
| "test": {}, | ||
| "test:coverage": {}, | ||
| "test": { | ||
| "dependsOn": ["^build"] |
There was a problem hiding this comment.
do we really need to depend on build for unit tests? usually unit test shouldn't rely on build as the tests are only at unit level
| "dependsOn": ["^build"] | ||
| }, | ||
| "test:coverage": { | ||
| "dependsOn": ["^build"] |
There was a problem hiding this comment.
same comment about build dependency as test script
| "cypress:ci": "START_SERVER_AND_TEST_INSECURE=1 start-server-and-test start https-get://localhost:3000 cypress:run", | ||
| "test": "jest --runInBand --detectOpenHandles --forceExit" | ||
| "test": "jest --runInBand --detectOpenHandles --forceExit", | ||
| "test:coverage": "pnpm test --coverage" |
There was a problem hiding this comment.
can you confirm if this work when running at project level? test seems to be halting at the end of test run the last time I tried
There was a problem hiding this comment.
I’m using supertest for integration testing, so the server has to fully initialize before tests run. On Windows starting up Flowise is very slow, which is why I added a 3-minute wait to avoid flaky failures. The tests aren’t truly stuck/halt, they’re waiting for startup to complete.
https://github.com/FlowiseAI/Flowise/blob/main/packages/server/test/index.test.ts
There was a problem hiding this comment.
maybe we should separate this out and use test:integration (or test:int, test:int:coverage for example) instead?
There was a problem hiding this comment.
I agree with splitting the test command.
For now, I’ll close this PR first since I’m still new to writing test scripts, and this test is only verifying a simple “Hello World”.
Also, we may not be using supertest in the future, so I’d prefer not to introduce new integration-test scripts until we decide on the longer-term testing approach.
3 participants
pnpm testpnpm test:coverage