fix: better error message for missing artifacts #973
Conversation
📝 WalkthroughWalkthroughThis pull request refactors artifact registration across the system. The Changes
Sequence Diagram(s)sequenceDiagram
participant P as Processor (Deno/Python/Substantial/Wasm)
participant FS as FsContext
participant TG as Typegraph
P->>FS: register_artifacts(tg, entry, deps)
FS->>FS: Register main artifact (entry)
loop For each dependency in deps
FS->>FS: Call private register_artifact
end
FS-->>P: Return list of registered artifacts
P->>TG: Update artifact/dependency registration
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/runtimes/deno/inexisting_dep.py (1)
1-25: Add a brief docstring for clarity
Even though it’s a test helper, a concise docstring explaining that this TypeGraph function is expected to fail due to a missing dependency ensures better maintainability and clarity for future contributors.src/typegraph/core/src/utils/postprocess/substantial_rt.rs (1)
24-34: LGTM! Clean implementation with proper mutation handling.The implementation correctly handles the nested workflow structure while maintaining the same artifact registration pattern.
Consider extracting the workflow processing logic into a separate method for better readability:
impl SubstantialProcessor { + fn process_workflow( + &self, + tg: &mut Typegraph, + fs_ctx: &FsContext, + wf_description: &mut WorkflowDescription, + ) -> Result<(), crate::errors::TgError> { + let entrypoint = wf_description.file.clone(); + let deps = std::mem::take(&mut wf_description.deps); + wf_description.deps = fs_ctx.register_artifacts(tg, entrypoint, deps)?; + Ok(()) + }tests/artifacts/artifacts_test.ts (1)
207-225: LGTM! Comprehensive test for missing artifact scenario.The test properly verifies both the error throwing and the error message content, aligning with the PR objective.
Consider adding a test case for multiple missing artifacts to ensure the error message handles this scenario well:
Meta.test({ only: mode === "default", name: `Missing artifact (mode: ${mode})`, ...options, }, async (t) => { await t.should("fail on missing artifact", async () => { await assertRejects( async () => { await t.engine("runtimes/deno/inexisting_dep.py"); }, ); try { await t.engine("runtimes/deno/inexisting_dep.py"); assert(false, "should have thrown"); } catch (e) { assertStringIncludes(e.message, "no artifacts found for dependency"); } }); + await t.should("fail on multiple missing artifacts", async () => { + try { + await t.engine("runtimes/deno/multiple_missing_deps.py"); + assert(false, "should have thrown"); + } catch (e) { + assertStringIncludes(e.message, "no artifacts found for dependency"); + // Verify all missing dependencies are mentioned + assertStringIncludes(e.message, "dep1.py"); + assertStringIncludes(e.message, "dep2.py"); + } + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/typegraph/core/src/utils/artifacts.rs(1 hunks)src/typegraph/core/src/utils/postprocess/deno_rt.rs(1 hunks)src/typegraph/core/src/utils/postprocess/python_rt.rs(1 hunks)src/typegraph/core/src/utils/postprocess/substantial_rt.rs(1 hunks)src/typegraph/core/src/utils/postprocess/wasm_rt.rs(1 hunks)tests/artifacts/artifacts_test.ts(2 hunks)tests/runtimes/deno/inexisting_dep.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (7)
src/typegraph/core/src/utils/artifacts.rs (3)
12-18: Refactor enables multi-artifact registration effectively
Defining a single method that accepts both the entry artifact and its dependencies is a good architectural step toward streamlined artifact handling.
21-45: Consider returning the entry artifact in the result
Currently, the function appends only dependency artifacts to the returned list. If you would like the caller to have a complete record of all registered artifacts (including the entry), consider pushing the entry artifact as well to avoid confusion.
49-60: Private method effectively updates typegraph metadata
This helper method appears clean and logically consistent. The usage of a hash and size for each artifact is aligned with standard artifact tracking.src/typegraph/core/src/utils/postprocess/wasm_rt.rs (1)
37-37: Transition from single to multi-artifact registration looks correct
Adoptingregister_artifactsis consistent with the new interface. Passing an empty dependency list here aligns with the logic that no additional WASM artifacts are needed.src/typegraph/core/src/utils/postprocess/python_rt.rs (1)
33-35: LGTM! Clean implementation of consolidated artifact registration.The changes align with the new pattern of using
register_artifactsand maintain proper error propagation.Let's verify that the error message for missing artifacts is properly propagated:
✅ Verification successful
Let's gather a bit more detail on the implementation of
register_artifactsto verify its error propagation. Please run the following shell scripts:After reviewing the output, we can confirm whether the error message for missing artifacts is correctly propagated (for example, if the function returns a Result and leverages the
?operator or explicit error conversion).
Verification of Error Propagation in
register_artifacts
- The implementation in
src/typegraph/core/src/utils/artifacts.rscorrectly returns an error usingErrwhen no artifacts are found.- The error message ("no artifacts found for dependency ...") is appropriately propagated via the
?operator in the caller's context.Everything appears to be working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling in the register_artifacts implementation ast-grep --pattern 'fn register_artifacts($_, $_, $_) { $$$ Err($_) $$$ }'Length of output: 82
Script:
#!/bin/bash # Locate the definition of register_artifacts and show surrounding context for inspection of error handling rg -n "fn register_artifacts" --context 15 .Length of output: 2845
src/typegraph/core/src/utils/postprocess/deno_rt.rs (1)
31-33: LGTM! Consistent implementation with other processors.The changes maintain the same pattern of consolidated artifact registration while properly handling Deno-specific module data.
tests/artifacts/artifacts_test.ts (1)
7-12: LGTM! Clean import organization.The imports are properly organized and include all necessary assertion functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/artifacts/artifacts_test.ts (2)
208-222: Simplify error assertion logic.The test has redundant error checking logic. Consider combining the assertions into a single, more descriptive test.
Here's a suggested improvement:
Meta.test(`Missing artifact`, async (t) => { await t.should("fail on missing artifact", async () => { await assertRejects( async () => { await t.engine("runtimes/deno/inexisting_dep.py"); }, - ); - try { - await t.engine("runtimes/deno/inexisting_dep.py"); - assert(false, "should have thrown"); - } catch (e) { - assertStringIncludes(e.message, "no artifacts found for dependency"); - } + Error, + "no artifacts found for dependency", + "should throw an error when artifact dependency is missing" + ); }); });
208-222: Consider adding more test cases for comprehensive error handling.While the current test verifies basic error handling, consider adding test cases for:
- Multiple missing artifacts
- Partially missing artifacts
- Different types of artifact dependencies
This would ensure robust error handling across various scenarios.
Would you like me to help generate additional test cases to improve coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/artifacts/artifacts_test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (1)
tests/artifacts/artifacts_test.ts (1)
7-12: LGTM! Clean import organization.The new imports are well-structured and necessary for the enhanced error testing capabilities.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #973 +/- ##
=======================================
Coverage 77.85% 77.85%
=======================================
Files 160 160
Lines 19641 19641
Branches 1969 1969
=======================================
Hits 15291 15291
Misses 4329 4329
Partials 21 21 ☔ View full report in Codecov by Sentry. |
Migration notes
Summary by CodeRabbit
New Features
Refactor
Tests