-
Notifications
You must be signed in to change notification settings - Fork 0
chore: loading resources to run locally #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
spark/src/test/scala/ai/chronon/spark/test/LocalDataLoaderTest.scala (1)
Line range hint
1-76
: Consider adding test cases for error scenariosThe test coverage could be improved by adding test cases for:
- Missing resources
- Invalid file formats
- Empty directories
- Files with incorrect schemas
Would you like me to provide example test implementations for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
spark/src/test/scala/ai/chronon/spark/test/LocalDataLoaderTest.scala
(2 hunks)
🔇 Additional comments (1)
spark/src/test/scala/ai/chronon/spark/test/LocalDataLoaderTest.scala (1)
66-67
:
Verify directory structure and handle resource access
The directory resource loading has similar issues as the file loading, with additional directory-specific concerns.
Let's verify the test resources structure:
Consider adding these improvements:
- val resourceURI = getClass.getResource("/local_data_csv")
- val path = new File(resourceURI.getFile)
+ val resourceURL = Option(getClass.getResource("/local_data_csv"))
+ .getOrElse(throw new IllegalStateException("Required test directory not found"))
+ val path = new File(resourceURL.getFile)
+ require(path.exists() && path.isDirectory, "Test resource must be an accessible directory")
For better test isolation and JAR compatibility:
- Consider creating a temporary directory with test files for each test run
- Add explicit verification of the expected directory structure
- Document the required test resource structure in the test class
spark/src/test/scala/ai/chronon/spark/test/LocalDataLoaderTest.scala
Outdated
Show resolved
Hide resolved
….scala Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
PTAL @piyush-zlai - lmk if you have feedback. A small one. |
## Summary - Load local resources irrespective of where the tests are currently being run from. This allows us to run them from Intellij. - ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved test robustness by replacing hardcoded file paths with dynamic resource URI retrieval for loading test data. - **Tests** - Enhanced flexibility in test cases for locating resources, ensuring consistent access regardless of the working directory. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary - Load local resources irrespective of where the tests are currently being run from. This allows us to run them from Intellij. - ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved test robustness by replacing hardcoded file paths with dynamic resource URI retrieval for loading test data. - **Tests** - Enhanced flexibility in test cases for locating resources, ensuring consistent access regardless of the working directory. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary - Load local resources irrespective of where the tests are currently being run from. This allows us to run them from Intellij. - ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved test robustness by replacing hardcoded file paths with dynamic resource URI retrieval for loading test data. - **Tests** - Enhanced flexibility in test cases for locating resources, ensuring consistent access regardless of the working directory. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary - Load local resources irrespective of where the tests are currently being run from. This allows us to run them from Intellij. - ## Cheour clientslist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved test robustness by replacing hardcoded file paths with dynamic resource URI retrieval for loading test data. - **Tests** - Enhanced flexibility in test cases for locating resources, ensuring consistent access regardless of the working directory. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Checklist
Summary by CodeRabbit
Bug Fixes
Tests