Skip to content

[4.x] Correct DatabaseTenancyBootstrapperTest#1466

Merged
stancl merged 5 commits into
masterfrom
fix-db-bootstrapper-test
Jul 1, 2026
Merged

[4.x] Correct DatabaseTenancyBootstrapperTest#1466
stancl merged 5 commits into
masterfrom
fix-db-bootstrapper-test

Conversation

@lukinovec

@lukinovec lukinovec commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Test for DatabaseTenancyBootstrapper throwing an exception when DB_URL is set

The test meant to cover this set central.url before creating a tenant. That made the CreateDatabase job fail with a QueryException (it tried to use the URL host as a database name). The bootstrapper's check was never reached, and the exception that the test intended to cover wasn't ever thrown in the end. The test passed for the wrong reason.

Fixed by creating the tenant first, setting the url only after that, and asserting that tenancy()->initialize() throws the bootstrapper's specific exception.

Reference env variable that Laravel uses now (DB_URL) instead of DATABASE_URL

Changed all DATABASE_URL references in Tenancy to DB_URL. In Laravel 11, the DATABASE_URL env variable got renamed to DB_URL in config/database.php. The env var got renamed quite a while ago, so referencing DATABASE_URL in Tenancy's comments and tests was a bit confusing.

Minor cleanup

The harden prevents tenants from using the database of another tenant test now reads the central connection name from config('tenancy.database.central_connection') instead of hardcoding 'central' for consistency with the other tests.

Summary by CodeRabbit

  • Bug Fixes
    • Improved database tenant initialization reliability when a central database URL is configured.
    • Updated the app to read the database URL from DB_URL, ensuring consistent connection setup.
    • Strengthened tenant database safety checks so the connection falls back correctly if hardening is enabled.
    • Refined test coverage for both URL-present and URL-absent database configuration scenarios.

Set the url in the template connection config only after creating a tenant. Before, the url was set before creating a tenant, and because of that, the tenant couldn't be created in the first place. During CreateDatabase, a QueryException (`SQLSTATE[HY000] [1049] Unknown database 'bc.us-east-1.rds.amazonaws.com'`) was thrown, and the test didn't get to exercise the bootstrapper's code branch that should throw an exception if the db url is set
The env var got renamed in Laravel 11 to `DB_URL`
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Renames the DATABASE_URL environment variable reference to DB_URL in a bootstrapper comment and test setup. Updates the DatabaseTenancyBootstrapperTest to remove an unused import, fix a hardening assertion to use the config value, and rewrite the URL validation test as a parameterized dataset test.

Changes

DB_URL rename and test updates

Layer / File(s) Summary
DB_URL rename in comment and test env config
src/Bootstrappers/DatabaseTenancyBootstrapper.php, tests/TestCase.php
Inline comment updated to reference DB_URL and describe silent-fail behavior; central MySQL connection url config now reads from env('DB_URL').
Test updates for DB_URL and central_connection config
tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php
Removes unused QueryException import; hardening assertion now uses config('tenancy.database.central_connection'); replaces the DATABASE_URL exception test with a parameterized dataset test over ['abc.us-east-1.rds.amazonaws.com', null] that asserts Exception with "template connection must NOT have URL defined" is thrown only when URL is non-null.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

  • archtechx/tenancy#1459: Directly modifies DatabaseTenancyBootstrapper tests and central-connection handling around DB_URL and tenant hardening, the same areas changed here.

Poem

🐇 A URL by another name,
DB_URL now enters the game.
The test was rewritten, parameterized too,
No DATABASE_URL left in the queue.
Hop hop, the bootstrapper's comments are true! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is specific and accurately reflects the main change: fixing the DatabaseTenancyBootstrapper test.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-db-bootstrapper-test

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.37%. Comparing base (df4be2e) to head (59ab773).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1466      +/-   ##
============================================
+ Coverage     86.35%   86.37%   +0.02%     
  Complexity     1200     1200              
============================================
  Files           186      186              
  Lines          3524     3524              
============================================
+ Hits           3043     3044       +1     
+ Misses          481      480       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Other tests grab the central connection from the config instead of hardcoding 'central'. Doing the same in 'harden prevents tenants from using the database of another tenant' for consistency.
@lukinovec lukinovec marked this pull request as ready for review June 30, 2026 04:46

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/TestCase.php`:
- Line 134: The shared test fixture in TestCase::createApplication() should not
depend on env('DB_URL'), because DatabaseTenancyBootstrapper::bootstrap()
rejects a non-null connection URL and makes tenancy tests sensitive to ambient
.env values. Remove the default url assignment from the suite-level central
database config, or switch it to a test-only env var so the fixture stays
URL-free unless a specific test overrides database.connections.central.url
itself.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: f9285dfd-5bc3-4ef4-81f6-2c4578078a3b

📥 Commits

Reviewing files that changed from the base of the PR and between df4be2e and e75ac38.

📒 Files selected for processing (3)
  • src/Bootstrappers/DatabaseTenancyBootstrapper.php
  • tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php
  • tests/TestCase.php

Comment thread tests/TestCase.php
…ency

Decided to go with the variable assignment instead of inlining the config(...) call in the assertion since it's a bit more readable imo
@lukinovec lukinovec changed the title Correct DatabaseTenancyBootstrapperTest [4.x] Correct DatabaseTenancyBootstrapperTest Jun 30, 2026
@stancl

stancl commented Jul 1, 2026

Copy link
Copy Markdown
Member

The harden prevents tenants from using the database of another tenant test now reads the central connection name from config('tenancy.database.central_connection') instead of hardcoding 'central' for consistency with the other tests.

Is this based on a coderabbit suggestion? I'm pretty sure we almost always hardcode central in tests. Doesn't matter either way though.

@stancl stancl merged commit 17706f8 into master Jul 1, 2026
14 checks passed
@stancl stancl deleted the fix-db-bootstrapper-test branch July 1, 2026 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants