chore: eslint rule for blocking importing features from appstore, lib, prisma #23832
Conversation
WalkthroughReplaces a single path-based Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
hbjORbj
changed the title
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(2 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
🔇 Additional comments (2)
.eslintrc.js (2)
31-32: LGTM: clear tRPC restriction message.Message is explicit and actionable; good guidance for routing UI to the right place.
16-17: Block root imports tooSwitching from paths → patterns removed the explicit root block; '@calcom/app-store' and '@calcom/features' (no subpath) are now allowed. Add root entries.
Apply this diff:
- patterns: ["@calcom/app-store/*", "@calcom/features/*"], + patterns: [ + "@calcom/app-store", + "@calcom/app-store/*", + "@calcom/features", + "@calcom/features/*" + ],Search in this environment returned "No files were searched" — unable to confirm absence of bare-root imports; run a repo-wide search locally to verify.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| "no-restricted-imports": [ | ||
| "warn", | ||
| { | ||
| paths: ["@calcom/app-store"], |
There was a problem hiding this comment.
patterns alone is sufficient
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.eslintrc.js (1)
24-42: Duplicate rule key: the second no-restricted-imports overrides the first. Merge them.Only the last key in the object is applied, which downgrades severity and drops allowTypeImports and the tRPC block. Merge into a single rule and also block the @calcom/features root with a dev-facing message.
- "no-restricted-imports": [ - "error", - { - allowTypeImports: false, - patterns: [ - { - group: ["@calcom/trpc/*", "@trpc/*"], - message: - "tRPC imports are blocked in packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", - }, - ], - }, - ], - "no-restricted-imports": [ - "warn", - { - patterns: ["@calcom/features/*"], - }, - ], + "no-restricted-imports": [ + "error", + { + allowTypeImports: false, + patterns: [ + { + group: ["@calcom/trpc/*", "@trpc/*"], + message: + "tRPC imports are blocked in packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", + }, + { + group: ["@calcom/features", "@calcom/features/*"], + message: + "Features are not allowed in packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", + }, + ], + }, + ],Run to confirm there are no other duplicate rule keys for the same override:
#!/bin/bash # Detect duplicate "no-restricted-imports" keys within a single rules object of any override. rg -nP '(?s)files:\s*\[.*?\]\s*,\s*rules:\s*\{[^}]*"no-restricted-imports"[^}]*"no-restricted-imports"' -C2 .eslintrc.js
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(2 hunks)
🔇 Additional comments (1)
.eslintrc.js (1)
31-32: LGTM — message reads clearly and points to the right remediation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.eslintrc.js (1)
13-18: Make it actually block in lib/prisma: use error severity, add allowTypeImports, add guidance.The PR says “block,” but this is only a warning. Also, be explicit about type-only imports and provide actionable messages.
Apply this diff:
"no-restricted-imports": [ - "warn", + "error", { - patterns: ["@calcom/app-store", "@calcom/app-store/*", "@calcom/features", "@calcom/features/*"], + allowTypeImports: false, + patterns: [ + { + group: ["@calcom/app-store", "@calcom/app-store/*"], + message: "app-store code must not be imported into lib/prisma. Move shared contracts to packages/lib or introduce an API boundary.", + }, + { + group: ["@calcom/features", "@calcom/features/*"], + message: "features are not allowed in lib/prisma. Extract shareable code to packages/lib or create an API boundary.", + }, + ], }, ],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(2 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Production builds / Build Atoms
🔇 Additional comments (1)
.eslintrc.js (1)
30-37: LGTM: tRPC ban expanded correctly.Including both “@calcom/trpc” and “@trpc” (and their subpaths) at error level with a clear message is good.
E2E results are ready! |
hbjORbj
force-pushed
the
chore/eslint-rule
branch
from
ab9faab to
6549534
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.eslintrc.js (1)
12-16: Critical: app-store→features ban is shadowed by the later override; consolidate and make it error-level.Files under packages/app-store match both overrides; the later one replaces the earlier rule config for import/no-restricted-paths, dropping the features ban. Also, "block" implies error severity. Fix by removing app-store from the first override, pruning unrelated zones, and adding the features zone into the app-store override at error level.
Apply this diff:
@@ - // WARN: features must not be imported by app-store, prisma, or lib + // ERROR: features must not be imported by prisma (app-store handled below; lib handled in its override) { files: [ - "packages/app-store/**/*.{ts,tsx,js,jsx}", "packages/prisma/**/*.{ts,tsx,js,jsx}", - "packages/lib/**/*.{ts,tsx,js,jsx}", ], rules: { "import/no-restricted-paths": [ - "warn", + "error", { zones: [ - { - target: "./packages/features", - from: "./packages/app-store", - message: "Do not import packages/features from packages/app-store.", - }, { target: "./packages/features", from: "./packages/prisma", message: "Do not import packages/features from packages/prisma.", }, - { - target: "./packages/features", - from: "./packages/lib", - message: "Do not import packages/features from packages/lib.", - }, ], }, ], }, }, @@ - // ERROR: app-store must not import trpc + // ERROR: app-store must not import trpc or features { files: ["packages/app-store/**/*.{ts,tsx,js,jsx}"], rules: { "import/no-restricted-paths": [ "error", { zones: [ { target: "./packages/trpc", from: "./packages/app-store", message: "packages/app-store must not import packages/trpc. Move UI to apps/web/components/apps or introduce an API boundary.", }, + { + target: "./packages/features", + from: "./packages/app-store", + message: "Do not import packages/features from packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", + }, ], }, ], }, },Also applies to: 18-41, 69-83
🧹 Nitpick comments (2)
.eslintrc.js (2)
18-41: Minor: remove now-dead duplication for lib.After the consolidation, this override no longer targets lib; keeping a lib zone here would be misleading. You already have a dedicated lib override below.
- { - target: "./packages/features", - from: "./packages/lib", - message: "Do not import packages/features from packages/lib.", - },
10-10: Comment label should reflect error-level enforcement.Update WARN→ERROR to match the actual severity to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
.eslintrc.js (1)
69-83: Optional hardening: block alias imports (add no-restricted-imports) for packages/app-store.Keep import/no-restricted-paths, and also add no-restricted-imports to catch aliased/resolver-bypassed imports.
File: .eslintrc.js (rules block)
rules: { - "import/no-restricted-paths": [ + "import/no-restricted-paths": [ "error", { zones: [ @@ ], }, ], + "no-restricted-imports": [ + "error", + { + patterns: [ + { + group: ["@calcom/trpc", "@calcom/trpc/*", "@trpc", "@trpc/*"], + message: + "packages/app-store must not import trpc directly. Move UI to apps/web/components/apps or introduce an API boundary.", + }, + { + group: ["@calcom/features", "@calcom/features/*"], + message: + "Do not import @calcom/features in packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", + } + ], + }, + ], },Verify with (use npx/pnpm if eslint isn't global):
#!/bin/bash if command -v eslint >/dev/null 2>&1; then eslint --print-config packages/app-store/src/index.ts | jq '.rules["import/no-restricted-paths"], .rules["no-restricted-imports"]' eslint --print-config packages/lib/src/index.ts | jq '.rules["import/no-restricted-paths"]' eslint --print-config packages/prisma/src/index.ts | jq '.rules["import/no-restricted-paths"]' elif command -v npx >/dev/null 2>&1; then npx --yes eslint --print-config packages/app-store/src/index.ts | jq '.rules["import/no-restricted-paths"], .rules["no-restricted-imports"]' npx --yes eslint --print-config packages/lib/src/index.ts | jq '.rules["import/no-restricted-paths"]' npx --yes eslint --print-config packages/prisma/src/index.ts | jq '.rules["import/no-restricted-paths"]' elif command -v pnpm >/dev/null 2>&1; then pnpm dlx eslint --print-config packages/app-store/src/index.ts | jq '.rules["import/no-restricted-paths"], .rules["no-restricted-imports"]' pnpm dlx eslint --print-config packages/lib/src/index.ts | jq '.rules["import/no-restricted-paths"]' pnpm dlx eslint --print-config packages/prisma/src/index.ts | jq '.rules["import/no-restricted-paths"]' else echo "Install eslint or run with npx/pnpm to print effective config." exit 1 fi
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.eslintrc.js (3)
68-92: Critical: app-store features ban is overridden and not enforced. Merge into one error-level rule.The later app-store override replaces the earlier one, dropping the features restriction for app-store. Consolidate here and keep severity at error; also disallow type-only imports.
"no-restricted-imports": [ "error", { + allowTypeImports: false, patterns: [ { group: [ // Catch all relative paths into trpc "**/trpc", "**/trpc/*", // Catch alias imports "@calcom/trpc", "@calcom/trpc/*", "@trpc", "@trpc/*", ], message: "packages/app-store must not import trpc. Move UI to apps/web/components/apps or introduce an API boundary.", }, + { + group: [ + // Catch all relative paths into features + "**/features", + "**/features/*", + // Catch alias imports + "@calcom/features", + "@calcom/features/*", + ], + message: + "packages/app-store must not import features. Move UI to apps/web/components/apps or introduce an API boundary.", + }, ], }, ],
11-15: Scope this override to prisma and make it truly blocking (error + no type-only imports).Avoid duplicate/overlapping overrides and enforce “block” semantics.
- files: [ - "packages/app-store/**/*.{ts,tsx,js,jsx}", - "packages/prisma/**/*.{ts,tsx,js,jsx}", - "packages/lib/**/*.{ts,tsx,js,jsx}", - ], + files: ["packages/prisma/**/*.{ts,tsx,js,jsx}"], @@ - "no-restricted-imports": [ - "warn", + "no-restricted-imports": [ + "error", { + allowTypeImports: false, patterns: [ { group: [ @@ - message: "Avoid importing packages/features from app-store, prisma, or lib.", + message: "packages/prisma must not import features.", }, ], }, ],Also applies to: 18-35
39-63: Enforce “block” in lib (error severity) and prevent type-only imports.Current warn won’t fail CI; type-only imports can bypass the ban.
- "no-restricted-imports": [ - "warn", + "no-restricted-imports": [ + "error", { - patterns: [ + allowTypeImports: false, + patterns: [ { group: [ // Catch all relative paths into app-store "**/app-store", "**/app-store/*", // Catch all relative paths into features "**/features", "**/features/*", // Catch alias imports "@calcom/app-store", "@calcom/app-store/*", "@calcom/features", "@calcom/features/*", ], message: "packages/lib should not import app-store or features.", }, ], }, ],
🧹 Nitpick comments (1)
.eslintrc.js (1)
9-9: Update comments to reflect error severity (not WARN) for accuracy.Keep file comments aligned with rule behavior.
- // WARN: features must not be imported by app-store, prisma, or lib + // ERROR: features must not be imported by app-store, prisma, or lib @@ - // WARN: lib must not import app-store or features + // ERROR: lib must not import app-store or featuresAlso applies to: 37-37
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
dosubot
bot
added
the
🧹 Improvements
| // Catch all relative paths into features | ||
| "**/features", | ||
| "**/features/*", |
There was a problem hiding this comment.
This might throw for valid imports e.g., importing a folder named features within the same package.
But we have a good message + we have no choice other than relying on no-restricted-imports rule for now since the zones approach isn't supported in our TS version (@typescript-eslint only supports <5.4 but we're on 5.9)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.eslintrc.js (3)
35-61: Make it truly “blocking” for lib and disallow type-only imports.Severity is warn; change to error and add allowTypeImports: false.
files: ["packages/lib/**/*.{ts,tsx,js,jsx}"], rules: { "no-restricted-imports": [ - "warn", + "error", { patterns: [ { group: [ // Catch all relative paths into app-store "**/app-store", "**/app-store/*", // Catch all relative paths into features "**/features", "**/features/*", // Catch alias imports "@calcom/app-store", "@calcom/app-store/*", "@calcom/features", "@calcom/features/*", ], message: "@calcom/lib should not import @calcom/app-store or @calcom/features.", }, ], + allowTypeImports: false }, ], },
11-32: Bug: this override is shadowed for app-store files; features ban doesn’t apply.ESLint merges overrides “last one wins” per rule. The later app-store override (Lines 64-87) redefines no-restricted-imports and drops the features ban for app-store. Remove this whole block and fold “features” into the app-store override to truly block it.
Apply this diff to delete the redundant override:
- // WARN: features must not be imported by app-store or lib - { - files: ["packages/app-store/**/*.{ts,tsx,js,jsx}", "packages/lib/**/*.{ts,tsx,js,jsx}"], - rules: { - "no-restricted-imports": [ - "warn", - { - patterns: [ - { - group: [ - // Catch all relative paths into features - "**/features", - "**/features/*", - // Catch all alias imports - "@calcom/features", - "@calcom/features/*", - ], - message: "Avoid importing @calcom/features from @calcom/app-store or @calcom/lib.", - }, - ], - }, - ], - }, - },
66-86: Fold “features” ban into app-store override and forbid type-only imports.Make the rule truly blocking for both trpc and features; prevent type-only escapes.
"no-restricted-imports": [ "error", { - patterns: [ + patterns: [ { group: [ // Catch all relative paths into trpc "**/trpc", "**/trpc/*", // Catch alias imports "@calcom/trpc", "@calcom/trpc/*", "@trpc", "@trpc/*", ], message: "@calcom/app-store must not import trpc. Move UI to apps/web/components/apps or introduce an API boundary.", }, + { + group: [ + // Catch all relative paths into features + "**/features", + "**/features/*", + // Catch all alias imports + "@calcom/features", + "@calcom/features/*", + ], + message: + "Features are not allowed in packages/app-store. Move UI to apps/web/components/apps or introduce an API boundary.", + }, ], + allowTypeImports: false }, ],
🧹 Nitpick comments (4)
.eslintrc.js (4)
94-111: Block type-only imports in prisma too.Prevent escapes via
import type."no-restricted-imports": [ "error", { patterns: [ { group: [ // Catch all relative paths into features "**/features", "**/features/*", // Catch all alias imports "@calcom/features", "@calcom/features/*", ], message: "Avoid importing @calcom/features from @calcom/prisma.", }, ], + allowTypeImports: false }, ],
64-64: Nit: update the comment to reflect both trpc and features are blocked.- // ERROR: app-store must not import trpc + // ERROR: app-store must not import trpc or features
35-35: Optional: include ESM/CJS and TS variants in file globs.Covers mts/cts/mjs/cjs if present in the repo.
- files: ["packages/lib/**/*.{ts,tsx,js,jsx}"], + files: ["packages/lib/**/*.{ts,tsx,js,jsx,mts,cts,mjs,cjs}"],- files: ["packages/app-store/**/*.{ts,tsx,js,jsx}"], + files: ["packages/app-store/**/*.{ts,tsx,js,jsx,mts,cts,mjs,cjs}"],- files: ["packages/prisma/**/*.{ts,tsx,js,jsx}"], + files: ["packages/prisma/**/*.{ts,tsx,js,jsx,mts,cts,mjs,cjs}"],Also applies to: 64-64, 92-92
41-56: Heads‑up: broad “/features” can flag legitimate local folders.**You noted this trade‑off; once feasible, prefer import/no-restricted-paths with zones for precise, path‑based boundaries (no TS 5.x dependency). Keep this as follow‑up tech debt.
Would you like me to draft a zones-based config that mirrors these constraints for when the parser/resolver setup allows it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.eslintrc.js(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build API v1
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
- GitHub Check: Production builds / Build Docs
- GitHub Check: Production builds / Build API v2
- GitHub Check: Tests / Unit
…, prisma (#23832) * eslint rule * improve * fix * improve msg
…ata (#23934) * feat: add BillingCacheService with 1-hour TTL for team subscription data - Create BillingCacheService following CalendarsCacheService pattern - Use teamId-based cache keys with 1-hour TTL (3,600,000 ms) - Integrate caching into getBillingData method in BillingService - Add cache invalidation to all webhook handlers: - handleStripeSubscriptionDeleted - handleStripePaymentSuccess - handleStripePaymentFailed - handleStripePaymentPastDue - handleStripeCheckoutEvents - Add cache invalidation to cancelTeamSubscription method - Add RedisModule import to billing module - Add BillingCacheService to billing module providers - Add findTeamByPlatformBillingId method to OrganizationsRepository for cache invalidation Co-Authored-By: [email protected] <[email protected]> * refactor: implement BillingServiceCachingProxy pattern - Extract IBillingService interface with all public methods - Create BillingServiceCachingProxy that implements caching logic - Remove all caching logic from original BillingService - Simplify cache invalidation using billing.id = team.id - Update module to use proxy with proper dependency injection - Update controller to inject proxy interface - Remove unused BillingService import from controller This follows the proxy pattern requested in PR feedback, separating caching concerns from core billing logic for better maintainability. Co-Authored-By: [email protected] <[email protected]> * chore: add e2e for billing check * chore: eslint rule for blocking importing features from appstore, lib, prisma (#23832) * eslint rule * improve * fix * improve msg * chore: fix any types set by devin * fix: add mising expect in test * refactor: move cache methods into BillingServiceCachingProxy - Remove BillingCacheService abstraction as suggested by @keithwillcode - Move cache methods directly into proxy as private methods - Update proxy to inject RedisService directly - Move BillingData type to interface for better type safety - Remove BillingCacheService from module providers - Delete unused billing-cache.service.ts file This simplifies the architecture by removing unnecessary abstraction and follows standard caching proxy patterns. Co-Authored-By: [email protected] <[email protected]> * fix: test and legacy starter --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: [email protected] <[email protected]> Co-authored-by: Benny Joo <[email protected]> Co-authored-by: Morgan <[email protected]>
…ata (calcom#23934) * feat: add BillingCacheService with 1-hour TTL for team subscription data - Create BillingCacheService following CalendarsCacheService pattern - Use teamId-based cache keys with 1-hour TTL (3,600,000 ms) - Integrate caching into getBillingData method in BillingService - Add cache invalidation to all webhook handlers: - handleStripeSubscriptionDeleted - handleStripePaymentSuccess - handleStripePaymentFailed - handleStripePaymentPastDue - handleStripeCheckoutEvents - Add cache invalidation to cancelTeamSubscription method - Add RedisModule import to billing module - Add BillingCacheService to billing module providers - Add findTeamByPlatformBillingId method to OrganizationsRepository for cache invalidation Co-Authored-By: [email protected] <[email protected]> * refactor: implement BillingServiceCachingProxy pattern - Extract IBillingService interface with all public methods - Create BillingServiceCachingProxy that implements caching logic - Remove all caching logic from original BillingService - Simplify cache invalidation using billing.id = team.id - Update module to use proxy with proper dependency injection - Update controller to inject proxy interface - Remove unused BillingService import from controller This follows the proxy pattern requested in PR feedback, separating caching concerns from core billing logic for better maintainability. Co-Authored-By: [email protected] <[email protected]> * chore: add e2e for billing check * chore: eslint rule for blocking importing features from appstore, lib, prisma (calcom#23832) * eslint rule * improve * fix * improve msg * chore: fix any types set by devin * fix: add mising expect in test * refactor: move cache methods into BillingServiceCachingProxy - Remove BillingCacheService abstraction as suggested by @keithwillcode - Move cache methods directly into proxy as private methods - Update proxy to inject RedisService directly - Move BillingData type to interface for better type safety - Remove BillingCacheService from module providers - Delete unused billing-cache.service.ts file This simplifies the architecture by removing unnecessary abstraction and follows standard caching proxy patterns. Co-Authored-By: [email protected] <[email protected]> * fix: test and legacy starter --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: [email protected] <[email protected]> Co-authored-by: Benny Joo <[email protected]> Co-authored-by: Morgan <[email protected]>
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Logs of testing
throws
