Implement diff command logic #28
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Error Handling Fails When Command Succeeds
The test's error handling is flawed. If execSync unexpectedly succeeds (i.e., does not throw), the test manually throws an Error('diff should fail'). The catch block then attempts to access err.stdout on this manually thrown Error object, which lacks a stdout property. This leads to misleading test failures, as assertions on output (derived from err.stdout) will fail instead of clearly indicating that the diff command unexpectedly succeeded.
cli/tests/cli.test.ts#L255-L262
omniscript-core/cli/tests/cli.test.ts
Lines 255 to 262 in 35cf112
Comment bugbot run to trigger another review on this PR
Was this report helpful? Give feedback by reacting with 👍 or 👎
- Enhanced the test for the CLI diff command to provide clearer failure messages when the command unexpectedly succeeds. - Added checks to ensure that output is only validated if an actual error occurs, improving robustness of the test. - This change ensures that the test accurately reflects the expected behavior when comparing different files. ## Testing - Run `npm test` to verify that the updated tests function correctly.
- Modified the CI/CD workflow to trigger on pushes to branches matching the pattern 'codex/**', in addition to 'main' and 'develop'. - This change allows for better integration and testing of features developed in the 'codex' namespace. ## Testing - Ensure the CI/CD pipeline runs successfully on the specified branches.
- Rename eslint.config.js to eslint.config.mjs for ES6 imports - Add @eslint/js dependency for ESLint 9 compatibility - Configure proper ignores for dist/, node_modules/, coverage/, and .d.ts files - Add Node.js globals (console, process, __dirname, etc.) - Relax strict rules to warnings for existing codebase - Update pnpm-lock.yaml for new dependencies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
diffDocsto compare OSF documentsosf diffTesting
pnpm testhttps://chatgpt.com/codex/tasks/task_e_685d3ac92b24832bae9bd15b7c6c0a1f