Skip to content

test(ai-lighting): finish Scene→Look rename in test fixtures - #179

Merged
bbernstein merged 1 commit into
mainfrom
feature/scene-to-look-cleanup
Apr 30, 2026
Merged

bbernstein merged 1 commit into
mainfrom
feature/scene-to-look-cleanup

Conversation

@bbernstein

Copy link
Copy Markdown
Owner

Summary

  • Bring the AI lighting service tests in line with the Scene→Look rename: rename Look-name mock data and corresponding describe/test labels.
  • Fix the stale sceneId field in the generateCueSequence mock to lookId, matching the CueData type.
  • Preserve theatrical-context strings ('Act 1, Scene 1', lookDescription: 'Romantic scene') per the rename plan's Phase 6.4 — those refer to script scenes, not lighting state.

Test plan

  • npm test (all 653 tests pass)
  • npm run lint

🤖 Generated with Claude Code

Bring the AI lighting service tests in line with the Scene→Look rename:
- Rename Look-name mock data ('Romantic Scene' → 'Romantic Look', etc.)
  and the corresponding describe/test labels
- Fix the stale sceneId field in the generateCueSequence mock to lookId,
  matching the CueData type
- Preserve theatrical-context strings ('Act 1, Scene 1', lookDescription:
  'Romantic scene') per the rename plan's Phase 6.4 — those refer to
  script scenes, not lighting state

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 30, 2026 16:39
@claude

claude Bot commented Apr 30, 2026

Copy link
Copy Markdown

Code Review

Overview

This is a focused, test-only PR that completes the Scene→Look rename in tests/services/ai-lighting.test.ts. It has 26 additions and 26 deletions — a pure rename with one meaningful correctness fix.


What's Good

  • Focused scope: Every change is intentional and part of a coherent rename pass. No unrelated noise.
  • Correctness fix for sceneId → lookId: This is the most substantive change and it matters. The generateCueSequence mock had a stale sceneId: '0' field that didn't match the CueData type. Fixing it here prevents a subtle false-positive in the test.
  • Theatrical context strings correctly preserved: Keeping 'Act 1, Scene 1' (script scene, not lighting state) and similarly-scoped strings shows good judgment — it distinguishes between the lighting domain rename and the theatrical script language.
  • Variable renames (scene/scenes → look/looks): Makes local variable names consistent with the domain terminology, which will reduce reader confusion.
  • All 653 tests pass + lint clean: Nothing regresses.

Suggestions / Minor Notes

  • description: 'A romantic lighting scene' → 'A romantic lighting look': This was changed in the mock data, which is correct — it's describing a lighting state, not a theatrical scene. Just confirming this was intentional (it is consistent with the rename goal).

  • Consider a follow-up grep for any remaining sceneId stragglers in other test files: The sceneId fix here was in one mock, but if CueData uses lookId throughout the production type, it's worth a quick scan to confirm no other test fixtures carry stale sceneId references. Not a blocker for this PR, but worth a note.

    grep -r "sceneId" tests/
  • No test behavior changed: This is worth stating explicitly — the PR doesn't add, remove, or alter any assertions. All changes are to mock data values and describe/it label strings. Risk is very low.


Summary

Clean, low-risk PR. The sceneId → lookId fix is the only real correctness change and it's correct. Everything else is cosmetic alignment. Approve with confidence.

Copilot AI 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.

Pull request overview

Updates the AI lighting service test fixtures to fully reflect the ongoing Scene → Look rename, ensuring test data matches current types and naming.

Changes:

  • Renamed test labels and mock look names/descriptions from “Scene” to “Look” for generateLook and additive-look coverage.
  • Updated the generateCueSequence mock cue payload to use lookId (instead of stale sceneId) to match the expected cue shape.
  • Renamed the scenes fixture array to looks in the generateCueSequence test while preserving theatrical script-context strings (e.g., Act 1, Scene 1).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bbernstein

Copy link
Copy Markdown
Owner Author

All review comments have been addressed:

  • Copilot's review left no inline comments — only a summary, which described the changes accurately.
  • Claude review's optional follow-up (grep for stale sceneId references in other test files) was performed: no other occurrences found in src/ or tests/.

All checks are passing (Lint, Test, Type Check, Test Coverage, Build, Docker Build, Security Audit, MCP Server Validation, claude-review). mergeStateStatus is CLEAN and the PR is ready for merge.

@bbernstein
bbernstein merged commit dc261aa into main Apr 30, 2026
16 checks passed
@bbernstein
bbernstein deleted the feature/scene-to-look-cleanup branch April 30, 2026 18:02
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