Reduce per-framework coupling in internal/conventions/ and tests/ - #120
Merged
Merged
Conversation
…tests/ Implementing the Dagster framework surfaced two shared-file coupling points that every framework addition touches, even when the touched content is framework-exclusive: - internal/conventions/ grew by 3-4 constants per framework (Default<Framework>DBUser/Pass/Name), even though most were only ever read by that one framework's own package. - tests/ mixed framework-specific test functions into shared grab-bag files (bootstrap_frameworks_test.go, bootstrap_staged_frameworks_test.go, framework_defaults_test.go, framework_snapshot_test.go, framework_detection_test.go), so adding a framework meant editing files that also contained unrelated frameworks' tests. Part A: move framework-exclusive constants (DB credential defaults, a few tool-binary paths) out of internal/conventions/ into the owning framework's own package, verified single-consumer via grep before each move. Genuinely shared constants (Magento1/Magento2/MageOS DB defaults, admin credentials, paths, permissions) stay in conventions. Delete internal/conventions/frameworks.go (14 FrameworkXxx constants with zero real consumers) plus two other dead constants (BinWpCli, UserMagento). Decouple internal/cmd/db_credentials.go's Postgres-engine fallback from Django's constants by introducing generic DefaultPostgresDBUser/Name. Part B: reorganize tests/ so a test function exercising exactly one framework lives in that framework's own <subject>_<framework>_test.go file; functions that genuinely compare >=2 frameworks are rewritten as table-driven tests with framework-generic names instead of living in a shared grab-bag file under a misleadingly specific name. Fold Dagster's dedicated CA-mount/linked-hosts render test into the existing generic non-PHP-frameworks loop, closing a duplicate-coverage gap. Part C: fix Drupal's bootstrap to use the shared conventions.DefaultAdminUser/Password/Email convention instead of hardcoded admin/admin/admin@local.test - the one intentional behavior change here. Existing Drupal sites are unaffected (the password lives in their database, not in this code path); newly-bootstrapped ones get the standard Admin12345678$ password instead of admin. Adding a new framework no longer requires editing internal/conventions/ at all. Every moved test function was verified byte-for-byte identical to its pre-move body; test-count reconciliation confirms the only function-count changes are the two deliberate consolidations described above, with zero accidental coverage loss. Closes #119 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ddtcorex
force-pushed
the
feature/framework-isolation-conventions-tests
branch
from
August 7, 2026 07:02
7b9051a to
7cf226f
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Default<Framework>DBUser/Pass/Name, a few tool-binary paths) out of the sharedinternal/conventions/package into each owning framework's own package, verified single-consumer via grep before each move. Genuinely shared constants (Magento1/Magento2/MageOS DB defaults, admin credentials, paths, permissions) stay inconventions.internal/conventions/frameworks.go(14FrameworkXxxconstants, all with zero real consumers) plus two other dead constants (BinWpCli,UserMagento).tests/: single-framework test functions move out of shared grab-bag files (bootstrap_frameworks_test.go,bootstrap_staged_frameworks_test.go,framework_defaults_test.go,framework_detection_test.go) into that framework's own<subject>_<framework>_test.go. Functions that genuinely compare ≥2 frameworks are rewritten table-driven with framework-generic names instead.conventions.DefaultAdminUser/Password/Emailconvention instead of hardcodedadmin/admin/admin@local.test— the one intentional behavior change in this PR. Existing Drupal sites are unaffected (the password lives in their database, not in this code path); newly-bootstrapped ones get the standardAdmin12345678$password instead ofadmin.Closes #119
Test plan
go build ./...,go vet ./...,gofmt -s -l .cleango test ./...andmake test(lint + fmt-check + vet + Go tests + frontend tests + integration tests) pass.go)dagster/django); 5 Minor findings deferred to a follow-up cleanup issue (naming-collision risk between local and sharedDefaultDBUser/Pass/Name-shaped constants, a couple of now-tautological tests, minor import-group style drift, pre-existing dual test-file naming convention)