Bootstrap Unit Testing by azaleacolburn · Pull Request #1179 · Autodesk/synthesis

azaleacolburn · 2025-06-27T19:53:32Z

Description

This is a branch off of #1175 which I can actually edit to make fixes, since @ariesninjadev was working from a personal fork.

The tests are not my work, I have just fixed the formatting and typing issues.

JIRA Issue

Dhruv-0-Arora · 2025-06-30T18:08:17Z

+    // NOTE Unnecessary test that uses URL wrong
+    // test("supports URL and history API", () => {
+    //     globalThis.URL = vi.fn(() => ({
+    //         searchParams: {
+    //             get: vi.fn(),
+    //             set: vi.fn(),
+    //         },
+    //     }))
+    //
+    //     const mockHistory = {
+    //         replaceState: vi.fn(),
+    //         pushState: vi.fn(),
+    //     }
+    //     Object.defineProperty(window, "history", {
+    //         value: mockHistory,
+    //         configurable: true,
+    //     })
+    //
+    //     expect(window.history.replaceState).toBeDefined()
+    //     expect(window.history.pushState).toBeDefined()
+    // })


AlexD717 · 2025-06-30T19:39:55Z

It appears that you have multiple unit tests that test practically the same thing, particularly in main.test.tsx.

BrandonPacewic

This might be the first case of AI "slop" within the Synthesis codebase, or at least in a PR. Going to combine a bunch of these tests into a single file as I'm seeing a lot of overlap here, should significantly cut down on the number of lines added by this PR.

AlexD717

I still feel that there are some tests in Synthesis.test.tsx that could be merged into one test

AlexD717

You are calling expect(screen.getByTestId("main-hud")).toBeDefined() in 5 tests. This is probably unnecessary and I would remove it from all tests but the first one. (A very similar thing is happening with screen.getByTestId("scene").

itspvty · 2025-07-09T17:35:34Z

You are calling expect(screen.getByTestId("main-hud")).toBeDefined() in 5 tests. This is probably unnecessary and I would remove it from all tests but the first one. (A very similar thing is happening with screen.getByTestId("scene").

Seems like these calls are part of verifying the outcome of the previously executed steps in each test?

AlexD717 · 2025-07-09T17:49:29Z

You are calling expect(screen.getByTestId("main-hud")).toBeDefined() in 5 tests. This is probably unnecessary and I would remove it from all tests but the first one. (A very similar thing is happening with screen.getByTestId("scene").

Seems like these calls are part of verifying the outcome of the previously executed steps in each test?

In some test it is verifying that the previous steps are correct. However, your third test renders the same exact thing as the first test, with no custom parameters or changes, and then verifies what was already verified in the first test. It verifies a new thing (that container.firstChild is defined) but it doesn't need to very that scene or main hud was defined.

rutmanz

This might be the first case of AI "slop" within the Synthesis codebase, or at least in a PR. Going to combine a bunch of these tests into a single file as I'm seeing a lot of overlap here, should significantly cut down on the number of lines added by this PR.

Strongly agree with this, it looks like a lot of the tests were created without intention and don't actually test the important parts

rutmanz · 2025-07-11T19:36:57Z

+        gtag: Mock
+        URL: Mock & typeof URL
+        testProperty: string


Why is anything being globally typed as Mock? I think it's fine to type as Function but vitest stuff should not be put in places that make it relevant to the rest of the codebase

rutmanz · 2025-07-11T19:54:25Z

+        expect(typeof window.convertAuthToken).toBe("function")
+        expect(typeof globalThis.gtag).toBe("function")


Aren't you defining both of these as mocks and they will therefore always be defined as functions?

rutmanz · 2025-07-11T19:58:08Z

+        )
+
+        expect(screen.getByTestId("test-component")).toBeDefined()
+        expect(screen.getByTestId("context-test")).toBeDefined()


This tests that they don't error but doesn't test that they're actually providing any context/they're doing what they're expected to do, so I'm not entirely sure what this test is helpful for

rutmanz · 2025-07-11T19:59:09Z

+        // The theme provider should set CSS custom properties on document root
+        // We can't easily test this in JSDOM, but we can verify the component renders
+        expect(screen.getByTestId("test-component")).toBeDefined()


???

So why does this test exist?

rutmanz · 2025-07-11T20:01:40Z

+        expect(() => {
+            render(
+                <ThemeProvider initialThemeName="Default" themes={mockThemes} defaultTheme={mockTheme}>
+                    <TestComponent />
+                </ThemeProvider>
+            )
+        }).not.toThrow()


rutmanz · 2025-07-11T20:06:22Z

+            )
+        }).not.toThrow()
+
+        // Test with empty theme object


I don't think this is actually a useful test to have if this isn't a scenario that ever occurs. It is, in my opinion, not useful to test edge cases by just checking for an error if those edge cases won't come up, and you can check if they do by just checking if the main app renders

rutmanz · 2025-07-11T20:10:57Z

+    },
+}))
+
+vi.mock("@/components/Scene.tsx", () => ({


Why are all of the components and other relevant files being mocked? Shouldn't we be testing that they all load and render without error?

BrandonPacewic

This branch needs to be manually updated as there were naming changes that are not reflected in git conflicts that are causing the tests to fail.

azaleacolburn · 2025-07-15T20:30:16Z

I'm going to take over and clean up this PR (@ariesninjadev learned his lesson apparently)

azaleacolburn · 2025-07-15T20:37:17Z

After further consideration, I don't actually think there's much of value in this PR at all. I'm going to close it and start fresh on this ticket.

azaleacolburn requested review from a team as code owners June 27, 2025 19:53

azaleacolburn requested review from BrandonPacewic, Dhruv-0-Arora and PepperLola June 27, 2025 19:53

azaleacolburn mentioned this pull request Jun 27, 2025

Bootstrap Unit Test Suite #1175

Closed

azaleacolburn force-pushed the aries/1942/bootstrap-testing branch 2 times, most recently from 68c3709 to 06b2bc7 Compare June 27, 2025 19:55

Dhruv-0-Arora assigned azaleacolburn and itspvty Jun 27, 2025

Dhruv-0-Arora added the testing Relates to testing; anywhere from unit testing to E2E label Jun 27, 2025

Dhruv-0-Arora approved these changes Jun 30, 2025

View reviewed changes

Dhruv-0-Arora reviewed Jun 30, 2025

View reviewed changes

BrandonPacewic requested changes Jul 1, 2025

View reviewed changes

Comment thread fission/src/test/bootstrap/main.test.tsx Outdated

BrandonPacewic reviewed Jul 3, 2025

View reviewed changes

AlexD717 requested changes Jul 7, 2025

View reviewed changes

itspvty requested review from AlexD717 and BrandonPacewic July 9, 2025 17:04

AlexD717 requested changes Jul 9, 2025

View reviewed changes

itspvty requested a review from AlexD717 July 9, 2025 20:08

rutmanz requested changes Jul 11, 2025

View reviewed changes

BrandonPacewic requested changes Jul 14, 2025

View reviewed changes

Dhruv-0-Arora self-requested a review July 14, 2025 20:23

itspvty and others added 4 commits July 15, 2025 13:25

Bootstrap tests round 1

7b8712e

Improvements to UTs

5da7c30

chore: bun run format

32b77a1

fix: globalThis typing

5e82aad

azaleacolburn and others added 8 commits July 15, 2025 13:25

fix: window and children typing issues

d3c612e

chore: remove window interface

5e93602

chore: formatting

98fc060

fix: build errors

469d7a7

fix: build

b6600a5

test(bootstrap): reduce main application bootstrap test

9998546

Remove duplicate tests

1f32801

Minor reductions

2e28c98

azaleacolburn force-pushed the aries/1942/bootstrap-testing branch from fb97509 to 2e28c98 Compare July 15, 2025 20:26

azaleacolburn closed this Jul 15, 2025

This was referenced Jul 16, 2025

Scene System Testing [AARD-1938] #1217

Merged

Bootstrap Unit Tests [AARD-1942] #1226

Merged

		expect(typeof window.convertAuthToken).toBe("function")
		expect(typeof globalThis.gtag).toBe("function")

Uh oh!

Conversation

azaleacolburn commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexD717 commented Jun 30, 2025

Uh oh!

Uh oh!

BrandonPacewic left a comment

Choose a reason for hiding this comment

Uh oh!

AlexD717 left a comment

Choose a reason for hiding this comment

Uh oh!

AlexD717 left a comment

Choose a reason for hiding this comment

Uh oh!

itspvty commented Jul 9, 2025

Uh oh!

AlexD717 commented Jul 9, 2025

Uh oh!

rutmanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrandonPacewic left a comment

Choose a reason for hiding this comment

Uh oh!

azaleacolburn commented Jul 15, 2025

Uh oh!

azaleacolburn commented Jul 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

azaleacolburn commented Jun 27, 2025 •

edited

Loading