test: reload config when imported file changes#34
Conversation
📝 WalkthroughWalkthroughAdds an integration test that starts the dev server with a temporary configuration importing another file, waits for the initial build, modifies the imported file, and verifies the expected server restart log within a 30-second timeout. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rstack/test/config/reload-app-config/index.test.ts`:
- Around line 52-75: Update the test case around “should reload config when an
imported file changes” to remove test-temp-import.config.ts and
test-temp-imported.ts before setup, then wrap execution in a finally block that
deletes both files after completion or failure. Follow the existing cleanup
pattern from the first test and preserve the current watcher assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da86d4a5-6097-4a87-8c69-72f896e1ed09
📒 Files selected for processing (1)
packages/rstack/test/config/reload-app-config/index.test.ts
| test('should reload config when an imported file changes', async ({ execCliAsync, logHelper }) => { | ||
| const configFile = path.join(import.meta.dirname, 'test-temp-import.config.ts'); | ||
| const importedFile = path.join(import.meta.dirname, 'test-temp-imported.ts'); | ||
|
|
||
| await writeFile(importedFile, ''); | ||
| await writeFile( | ||
| configFile, | ||
| `import { define } from 'rstack'; | ||
| import './test-temp-imported.ts'; | ||
|
|
||
| define.app({ | ||
| server: { port: ${await getRandomPort()} }, | ||
| }); | ||
| `, | ||
| ); | ||
|
|
||
| execCliAsync('dev --config test-temp-import.config.ts'); | ||
| await logHelper.expectBuildEnd(); | ||
| logHelper.clearLogs(); | ||
|
|
||
| await writeFile(importedFile, '// changed\n'); | ||
|
|
||
| await logHelper.expectLog('restarting server as test-temp-imported.ts changed'); | ||
| }, 30_000); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add cleanup of temp files before and after the test.
The new test creates test-temp-import.config.ts and test-temp-imported.ts but never removes them. The first test in this file cleans up its temp files at the start (lines 12–14); this test should follow the same pattern and also clean up after the test to avoid leaving artifacts on success or failure. Stale files from a failed previous run could interfere with file watchers or import resolution.
🧹 Proposed fix: add cleanup at start and in finally block
test('should reload config when an imported file changes', async ({ execCliAsync, logHelper }) => {
const configFile = path.join(import.meta.dirname, 'test-temp-import.config.ts');
const importedFile = path.join(import.meta.dirname, 'test-temp-imported.ts');
+ await rm(configFile, { force: true });
+ await rm(importedFile, { force: true });
+
+ try {
await writeFile(importedFile, '');
await writeFile(
configFile,
`import { define } from 'rstack';
import './test-temp-imported.ts';
define.app({
server: { port: ${await getRandomPort()} },
});
`,
);
execCliAsync('dev --config test-temp-import.config.ts');
await logHelper.expectBuildEnd();
logHelper.clearLogs();
await writeFile(importedFile, '// changed\n');
await logHelper.expectLog('restarting server as test-temp-imported.ts changed');
+ } finally {
+ await rm(configFile, { force: true });
+ await rm(importedFile, { force: true });
}
}, 30_000);📝 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('should reload config when an imported file changes', async ({ execCliAsync, logHelper }) => { | |
| const configFile = path.join(import.meta.dirname, 'test-temp-import.config.ts'); | |
| const importedFile = path.join(import.meta.dirname, 'test-temp-imported.ts'); | |
| await writeFile(importedFile, ''); | |
| await writeFile( | |
| configFile, | |
| `import { define } from 'rstack'; | |
| import './test-temp-imported.ts'; | |
| define.app({ | |
| server: { port: ${await getRandomPort()} }, | |
| }); | |
| `, | |
| ); | |
| execCliAsync('dev --config test-temp-import.config.ts'); | |
| await logHelper.expectBuildEnd(); | |
| logHelper.clearLogs(); | |
| await writeFile(importedFile, '// changed\n'); | |
| await logHelper.expectLog('restarting server as test-temp-imported.ts changed'); | |
| }, 30_000); | |
| test('should reload config when an imported file changes', async ({ execCliAsync, logHelper }) => { | |
| const configFile = path.join(import.meta.dirname, 'test-temp-import.config.ts'); | |
| const importedFile = path.join(import.meta.dirname, 'test-temp-imported.ts'); | |
| await rm(configFile, { force: true }); | |
| await rm(importedFile, { force: true }); | |
| try { | |
| await writeFile(importedFile, ''); | |
| await writeFile( | |
| configFile, | |
| `import { define } from 'rstack'; | |
| import './test-temp-imported.ts'; | |
| define.app({ | |
| server: { port: ${await getRandomPort()} }, | |
| }); | |
| `, | |
| ); | |
| execCliAsync('dev --config test-temp-import.config.ts'); | |
| await logHelper.expectBuildEnd(); | |
| logHelper.clearLogs(); | |
| await writeFile(importedFile, '// changed\n'); | |
| await logHelper.expectLog('restarting server as test-temp-imported.ts changed'); | |
| } finally { | |
| await rm(configFile, { force: true }); | |
| await rm(importedFile, { force: true }); | |
| } | |
| }, 30_000); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rstack/test/config/reload-app-config/index.test.ts` around lines 52
- 75, Update the test case around “should reload config when an imported file
changes” to remove test-temp-import.config.ts and test-temp-imported.ts before
setup, then wrap execution in a finally block that deletes both files after
completion or failure. Follow the existing cleanup pattern from the first test
and preserve the current watcher assertions.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7195f5adc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await writeFile( | ||
| configFile, | ||
| `import { define } from 'rstack'; | ||
| import './test-temp-imported.ts'; |
There was a problem hiding this comment.
Because the imported file is only loaded for side effects and the assertion only waits for the restart log, this test can pass even if the dev server notices the dependency but reloads the config with stale imported values. In the scenario this is meant to cover—an imported config helper changes—the regression would still slip through; make the imported module export an observable config value, change it, and assert the post-restart build/server reflects the new value.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR adds a minimal integration test for config dependency watching. It changes a file imported by the Rstack config and verifies that the development server reloads.
Related Links