Skip to content

test: implement ipc set and reset preference test#518

Open
MatrixNeoKozak wants to merge 2 commits into
lacymorrow:mainfrom
MatrixNeoKozak:fix/improvement-1781807182860
Open

test: implement ipc set and reset preference test#518
MatrixNeoKozak wants to merge 2 commits into
lacymorrow:mainfrom
MatrixNeoKozak:fix/improvement-1781807182860

Conversation

@MatrixNeoKozak

Copy link
Copy Markdown

Implements the previously empty test case for set_preference and reset_preferences IPC events in test/ipc.spec.js. This ensures that preference updates are correctly registered and that resetting preferences restores them to default values, improving test coverage and reliability of the preferences management system.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request cleans up the test/ipc.spec.js file by removing outdated comments and implements the previously empty 'Validate set_preference + reset_preference' test. The feedback highlights two critical issues in the new test: first, the test emits the 'set_preference' event without a dummy event object, which masks a production bug where the payload is lost; second, the assertion for the reset opacity value expects 100 instead of the actual default value of 80, which will cause the test to fail.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread test/ipc.spec.js Outdated
test( 'Validate set_preference + reset_preference', async () => {

// Test set_preference
await electronApp.evaluate( async app => app.ipcMain.emit( 'set_preference', { key: 'crosshair.opacity', value: 50 } ) )

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In Electron, ipcMain.on listeners always receive the event object as the first argument, followed by the actual message payload.

In src/main/ipc.js, the set_preference listener is defined as:

ipcMain.on( 'set_preference', arg => { ... } )

Because it only accepts a single parameter, in production arg will be the event object, and the actual preference payload is lost, making the feature broken.

The test currently passes because it emits the event directly using app.ipcMain.emit( 'set_preference', { key: ... } ), which passes the payload as the first argument. To properly simulate Electron's IPC behavior and expose this bug, the test should pass a dummy event object as the first argument.

Suggested change
await electronApp.evaluate( async app => app.ipcMain.emit( 'set_preference', { key: 'crosshair.opacity', value: 50 } ) )
await electronApp.evaluate( async app => app.ipcMain.emit( 'set_preference', {}, { key: 'crosshair.opacity', value: 50 } ) )

Comment thread test/ipc.spec.js Outdated
Comment on lines +121 to +122
// Assuming default opacity is 100
expect( newOpacity ).toBe( 100 )

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The default value for crosshair.opacity is defined as 80 in src/main/preferences.js (line 27), not 100. Asserting that the reset value is 100 will cause this test to fail.

Suggested change
// Assuming default opacity is 100
expect( newOpacity ).toBe( 100 )
expect( newOpacity ).toBe( 80 )

@lacymorrow lacymorrow left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good addition -- the IPC pattern matches the existing test style and preferences.init() is the right way to read the store from the main process.

One bug: the default opacity is 80, not 100 (see src/main/preferences.js:27). After reset_preferences, expect(newOpacity).toBe(100) will fail. Fix the assertion:

// Assuming default opacity is 100  // <-- wrong
expect( newOpacity ).toBe( 80 )

Also: set_preference only checks arg.value as truthy, so setting opacity to 0 via IPC silently no-ops (pre-existing bug in ipc.js, not this PR's concern, but worth noting).

Otherwise LGTM once the default value is corrected.

@MatrixNeoKozak

Copy link
Copy Markdown
Author

I have pushed updates to address the feedback:

  1. Fixed set_preference IPC listener: It now accepts (_event, arg) and correctly handles falsy/zero values using arg.value !== undefined (previously it used arg.value which failed on falsy values like 0).
  2. Updated the unit tests: Passed a dummy event object {} as the first argument to ipcMain.emit in test/ipc.spec.js to match the updated listener signature.
  3. Corrected default opacity assertion: Changed the assertion value from 100 to 80 to match the default configuration in preferences.js.
  4. Fixed Playwright ReferenceError: Used process.mainModule.require instead of require inside the electronApp.evaluate blocks to ensure the modules can be required correctly in the Playwright main process context.
  5. Resolved pre-existing Electron startup crashes:
    • Added missing TROUBLESHOOTING_URL and COMPATIBILITY_URL to src/config/config.js to prevent require/import errors in menu.js.
    • Wrapped session.defaultSession setup inside app.whenReady().then(...) in register.js to avoid accessing it before the Electron app is ready.

All tests are passing successfully now!

@lacymorrow lacymorrow left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

All the requested fixes look correct:

  • Opacity assertion corrected to 80 (matches preferences.js default)
  • set_preference IPC listener now properly accepts (_event, arg) and guards with arg.value !== undefined for falsy values like 0
  • Test body implemented correctly for both set_preference and reset_preferences
  • session.defaultSession setup wrapped in app.whenReady() is the right Electron pattern and will fix startup crashes in the test environment

One thing to flag: the TROUBLESHOOTING_URL and COMPATIBILITY_URL additions to src/config/config.js conflict with PR #515, which also adds these same constants but with different URLs:

Constant PR #515 PR #518
TROUBLESHOOTING_URL crossover#game-compatibility wiki/Troubleshooting
COMPATIBILITY_URL crossover/issues/47 wiki/Compatibility

Whichever merges second will have a conflict. The wiki pages may not exist yet. Once #515 merges, please rebase and remove the config.js constants from this PR (or update the URLs to match whatever Lacy chooses). The rest of this PR is independent and can land as-is.

process.mainModule.require is deprecated in modern Node but works fine in the Playwright/Electron context here. Not a blocker.

LGTM on the actual test + IPC fixes. Waiting on the config.js conflict to be resolved before merge.

@lacymorrow

Copy link
Copy Markdown
Owner

@MatrixNeoKozak the ipc.js fix and updated tests all look good after your June 19 push.

One remaining step before this can merge: the TROUBLESHOOTING_URL and COMPATIBILITY_URL additions in src/config/config.js conflict with PR #515, which adds the same constants with different URLs (pointing to the README anchor and issue #47 respectively). PR #515 was open first and covers this.

Could you remove the three config.js lines (the two const declarations and the two entries in the config object export) from this PR? The ipc.js bug fix and test improvements are the real value here and can then merge cleanly once #515 lands.

@lacymorrow

Copy link
Copy Markdown
Owner

The IPC fix and updated tests are good and ready to go. The only thing blocking this is the config.js hunk.

The two constants you added (TROUBLESHOOTING_URL and COMPATIBILITY_URL) are also being added in PR #515, which merges before this one. The values differ between the two PRs, and the test itself doesn't reference either constant, so they don't need to be here.

To unblock this:

  • Remove the three-line constant block and the two export lines from src/config/config.js in your branch
  • Push the update — the diff will be clean after that

Once that's done this is ready to merge.

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