Skip to content

Scene System Testing [AARD-1938] - #1217

Merged
BrandonPacewic merged 20 commits into
devfrom
alexey/1938/scene-system-testing
Jul 21, 2025
Merged

Scene System Testing [AARD-1938]#1217
BrandonPacewic merged 20 commits into
devfrom
alexey/1938/scene-system-testing

Conversation

@AlexD717

@AlexD717 AlexD717 commented Jul 15, 2025

Copy link
Copy Markdown
Member

Task

Create unit tests for each element found in fission/src/systems/scene/.

AARD-1938

Symptom

No unit tests currently exist for elements in fission/src/systems/scene/.

Solution

Create unit test for each element in fission/src/systems/scene/.
Note: No unit tests were created for SceneObject.ts due to it being very simple and other tests already testing its functionality.

Verification

All unit tests pass


Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@AlexD717 AlexD717 self-assigned this Jul 15, 2025
@AlexD717
AlexD717 requested review from a team as code owners July 15, 2025 21:30
@AlexD717 AlexD717 added the testing Relates to testing; anywhere from unit testing to E2E label Jul 15, 2025
@AlexD717
AlexD717 marked this pull request as draft July 15, 2025 21:35
@AlexD717
AlexD717 marked this pull request as ready for review July 15, 2025 21:53

@rutmanz rutmanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm only about halfway done reading through this but I'm concerned that a lot of the tests here aren't really checking for things that matter. Like #1179, it kind of feels like these might be AI generated, and I'm not sure what a lot of them are doing

Comment thread fission/src/test/scene/CameraControls.test.ts Outdated
Comment thread fission/src/test/scene/CameraControls.test.ts Outdated
Comment thread fission/src/test/scene/DragModeSystem.test.ts
Comment thread fission/src/test/scene/DragModeSystem.test.ts Outdated
Comment thread fission/src/test/scene/GizmoSceneObject.test.ts Outdated
Comment thread fission/src/test/scene/GizmoSceneObject.test.ts Outdated
Comment thread fission/src/test/scene/GizmoSceneObject.test.ts Outdated
@AlexD717

Copy link
Copy Markdown
Member Author

I'm only about halfway done reading through this but I'm concerned that a lot of the tests here aren't really checking for things that matter. Like #1179, it kind of feels like these might be AI generated, and I'm not sure what a lot of them are doing

I did use AI to help make these unit tests, and I thought that I looked through them and fixed all the mistakes. I have clearly missed some, so I will go through again and double check that everything is good.

@AlexD717
AlexD717 marked this pull request as draft July 16, 2025 01:22
@AlexD717
AlexD717 marked this pull request as ready for review July 17, 2025 23:04
@AlexD717
AlexD717 requested a review from rutmanz July 17, 2025 23:05

public setTargetCoordinates(coords: Partial<SphericalCoords>) {
if (coords.theta !== undefined) this._nextCoords.theta = coords.theta
if (coords.phi !== undefined) this._nextCoords.phi = coords.phi
if (coords.r !== undefined) this._nextCoords.r = coords.r
}

public setImmediateCoordinates(coords: Partial<SphericalCoords>) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are we removing this function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This function was added in the view cube pr, but it isn't actually used anywhere in the code. It also only moves the camera towards the target coordinates for only one frame and then stops.

Comment thread fission/src/test/scene/SceneInteractionHandler.test.ts
Comment thread fission/src/test/scene/DragModeSystem.test.ts Outdated
@AlexD717
AlexD717 requested a review from Dhruv-0-Arora July 18, 2025 16:47
Comment thread fission/src/test/scene/DragModeSystem.test.ts
Comment thread fission/src/test/scene/SceneRenderer.test.ts Outdated
Comment thread fission/src/test/scene/SceneRenderer.test.ts Outdated
Comment thread fission/src/test/scene/SceneRenderer.test.ts Outdated
Comment thread fission/src/test/scene/SceneRenderer.test.ts
Comment thread fission/src/test/scene/SceneRenderer.test.ts Outdated
@AlexD717
AlexD717 requested a review from rutmanz July 18, 2025 22:39

@ryanzhangofficial ryanzhangofficial 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.

Looks great!

@BrandonPacewic
BrandonPacewic merged commit 278177e into dev Jul 21, 2025
16 checks passed
@BrandonPacewic
BrandonPacewic deleted the alexey/1938/scene-system-testing branch July 21, 2025 16:16
@BrandonPacewic BrandonPacewic mentioned this pull request Jul 21, 2025
PepperLola pushed a commit that referenced this pull request Jul 25, 2025
Co-authored-by: Alexey Dmitriev <157652245+AlexD717@users.noreply.github.com>
Co-authored-by: Brandon Pacewic <92102436+BrandonPacewic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Relates to testing; anywhere from unit testing to E2E

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants