Add system test infrastructure and critical path browser specs #54
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "52-add-system-test-infrastructure-and-criti"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Changes
Gemfile/Gemfile.lock: addedcapybaraandcupritegems to:testgroupspec/support/system_test_config.rb: new -- configures Cuprite driver with headless Chrome, no-sandbox for CI, CHROMIUM_PATH env var supportspec/rails_helper.rb: uncommented auto-require forspec/support/**/*.rbso system test config loads automaticallyspec/system/work_queue_spec.rb: new -- tests adding property to queue via quick-add form and toggling completion statespec/system/uploads_spec.rb: new -- tests upload page rendering, Stimulus controller wiring, and existing upload displayspec/system/properties_spec.rb: new -- tests active/inactive toggle via Turbo Stream on manage page (both directions).woodpecker.yaml: installschromiumin CI test step, setsCHROMIUM_PATHenv varspec/fixtures/files/test_image.jpg: test fixture for upload specTest Plan
bundle exec rspec spec/system/-- 7 examples, 0 failuresbundle exec rspec-- 86 examples, 0 failures (all existing specs unaffected)ruby:3.4.9-slimand runs system specs headlessReview Checklist
Related Notes
landscaping-assistant-- project boardQA Review
Scope: 9 files changed, +222/-7 lines. System test infrastructure + 3 critical-path browser specs.
Findings
No blocking issues found.
Positives:
rack_testas default driver withcupriteonly for system specs preserves fast non-JS test executionfill_in "client_name",.btn-complete,#manage_property_N,.queue-item,.is-completedall matchchromiumpackage andCHROMIUM_PATHenv var for theruby:3.4.9-slimimageMinor observations (non-blocking):
driven_by :cupritein the before hook is slightly redundant withCapybara.javascript_driver = :cupritesince system specs default to the JS driver. However, being explicit is defensible and matches Rails convention.Test Results
Per PR body: 7 system specs pass, 86 total specs pass, 0 failures.
VERDICT: APPROVE
PR #54 Review
DOMAIN REVIEW
Stack: Ruby on Rails 8.1 / RSpec / Capybara+Cuprite / Turbo Streams / Stimulus / Woodpecker CI
Infrastructure (Cuprite config --
spec/support/system_test_config.rb):no-sandbox,disable-gpu,disable-dev-shm-usage.CHROMIUM_PATHenv var support allows CI to override the browser binary path. Clean pattern.process_timeout: 15,timeout: 10) are reasonable for CI. Local dev gets the same defaults, which is fine.Capybara.default_driver = :rack_testpreserves fast non-JS specs; only system specs use Cuprite viadriven_by :cupritein the before block. Correct separation.Woodpecker CI (
.woodpecker.yaml):chromiumadded to apt-get inruby:3.4.9-slim. This is sufficient -- Cuprite uses Chrome DevTools Protocol directly, no separate chromedriver binary needed. Correct.CHROMIUM_PATH: /usr/bin/chromiumis the standard location in Debian-based slim images. Correct.Gem selection (
Gemfile):capybaraandcupritein:testgroup only. Noselenium-webdriver(correct -- not needed). Clean.rails_helper.rb change:
Selector verification against actual view templates:
All selectors in the specs were verified against the actual ERB templates:
#work_queue_item_#{id}_queue_item.html.erb:1--dom_id(item).queue-item_queue_item.html.erb:1.btn-complete_queue_item.html.erb:12.is-completed_queue_item.html.erb:1(conditional)#manage_property_#{id}manage.html.erb:27--dom_id(property, :manage)have_button("Active"/"Inactive")manage.html.erb:41+toggle_active.turbo_stream.erb"Show Inactive"manage.html.erb:19.upload-list-itemuploads/index.html.erb:18[data-controller='upload']uploads/index.html.erb:4input[data-action='change->upload#submit']uploads/index.html.erb:11label.upload-btnuploads/index.html.erb:8"No photos yet"uploads/index.html.erb:33(substring match)Spec quality:
client_nameand clicks "Add", which exercises theWorkQueueItemsController#createpath withparams[:client_name]. Correct.property.reload. Thorough.Test fixture:
spec/fixtures/files/test_image.jpgexists and is a valid JPEG. Size is reasonable for a test fixture.BLOCKERS
None.
NITS
use_transactional_fixtures = truewith system specs: Rails system specs with a JS driver (Cuprite) run the server in a separate thread. Transactional fixtures can cause the server thread to not see records created in the test transaction. This typically works in Rails 8.x because system tests usedriven_bywhich handles the threading, but if you ever see intermittent "record not found" failures in system specs, this is the first thing to check. The canonical fix is to addDatabaseCleanerwith truncation strategy for system specs, but given that all 7 examples pass locally, this is not blocking -- just something to watch for.Upload spec --
Upload.new+ manual attach pattern: Thedisplays existing uploadsexample manually constructs an Upload withUpload.new+photo.attach+save!. This is fine for a system spec, but a factory or shared helper would reduce duplication if more upload specs are added later. Not blocking for 1 usage.Missing
file_fixture_uploadusage: The upload spec usesFile.open(file_fixture(...))directly. For consistency with Rails conventions,file_fixture_upload("test_image.jpg", "image/jpeg")fromActionDispatch::TestProcess::FixtureFilecould be used instead. Minor style preference -- what's there works.SOP COMPLIANCE
52-add-system-test-infrastructure-and-criti(truncated but matches issue #52)landscaping-assistantproject board referencePROCESS OBSERVATIONS
VERDICT: APPROVED
Accuracy Audit:
docs/testing-strategy.mdEvery factual claim in the document was checked against the actual code in the worktree. Findings below.
Test Inventory: Example Counts
Model Specs -- Doc claims 18, actual is 19
itblocksThe Property model spec has 8 examples. The doc's description ("Validations, services association,
.activescope,#maps_url") is directionally correct but the count is wrong.Request Specs -- Doc claims 51, actual is 60
itblocksThe doc's sub-descriptions of endpoints covered are largely correct (the right routes are listed), but every per-controller count except Health and Client Errors is wrong. The Weeks doc says "index (7 scenarios), move (4 scenarios)" but actual is index: 9, move: 4. The doc says WorkQueueItems has "index, create, update, destroy, reorder" which matches the endpoint coverage, but claims 10 examples when there are 8.
System Specs -- Doc claims 7, actual is 7
itblocksGrand Total -- Doc claims 76, actual is 86
Doc: 18 + 51 + 7 = 76. Actual: 19 + 60 + 7 = 86. Off by 10.
Endpoints Covered Claims
Verified each request spec file against the doc's "Endpoints covered" column.
Gemfile Claims
gem "capybara"in test groupgem "cuprite"in test groupgroup :test doDoc does not mention specific versions, so no version mismatch to flag.
.woodpecker.yamlClaimschromiumviaapt-getalongsidelibpq-devandbuild-essentialapt-get install -y libpq-dev build-essential libyaml-dev chromium(note: also installslibyaml-dev, not mentioned in doc -- minor omission)CHROMIUM_PATH: /usr/bin/chromiumbundle exec rspec--fail-level=errorevent: push, branch: mainimage: postgres:16when: event: push, branch: mainanddepends_on: [lint, test]Cuprite Config (
spec/support/system_test_config.rb)no-sandbox,disable-gpu,disable-dev-shm-usageCHROMIUM_PATHenv varrack_testconfig.before(:each, type: :system) { driven_by :cuprite }Remaining Gaps / Not Planned
app/services/directory exists at all. Geocoding is entirely client-side JS.Model.create!directly.System Spec CSS Selectors
.queue-itemwork_queue_spec.rbline 18:have_css(".queue-item", text: ...)AND_queue_item.html.erbline 1:class="queue-item".is-completedwork_queue_spec.rblines 37, 41:have_css(".is-completed")AND_queue_item.html.erbline 1:'is-completed' if item.completed?.btn-completework_queue_spec.rbline 38:find(".btn-complete").clickAND_queue_item.html.erbline 12:class: "btn-icon btn-complete"#manage_property_properties_spec.rblines 18, 24, 42, 48:within("#manage_property_#{property.id}")ANDmanage.html.erbline 27:id="<%= dom_id(property, :manage) %>"(generatesmanage_property_N)#work_queue_item_work_queue_spec.rblines 36, 41:within("#work_queue_item_#{queue_item.id}")AND_queue_item.html.erbline 1:id="<%= dom_id(item) %>"(generateswork_queue_item_N)data-controller='upload'uploads_spec.rbline 25:have_css("[data-controller='upload']")ANDuploads/index.html.erbline 4:data: { controller: "upload" }data-action='change->upload#submit'uploads_spec.rbline 28:have_css("input[data-action='change->upload#submit']")ANDuploads/index.html.erbline 11:data: { action: "change->upload#submit" }Critical Path Coverage Tables
The per-step coverage descriptions in the "Critical User Paths" section are accurate. Each "Request spec" or "System spec" claim maps to a real spec. Each "Not tested" claim was confirmed (no matching spec exists). The resolve endpoint scenarios listed (creates property, finds existing, assigns services, TBD default, preserves existing name) all have corresponding
itblocks.One note: Path 2 mentions "Resolve defaults: Request spec: TBD default, preserves existing name" -- this maps to 3 actual
itblocks (TBD when no client_name, TBD when no street and no client_name, preserves existing name), not the 2 implied by the description.Summary of Inaccuracies
All inaccuracies are count mismatches in the Test Inventory section. No structural or coverage claims are wrong.
Everything else -- endpoint coverage, gem config, CI pipeline, Cuprite setup, CSS selectors, remaining gaps, not-planned rationale -- is accurate.