Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better Mock updates #269

Merged
merged 42 commits into from
Nov 30, 2023
Merged

Better Mock updates #269

merged 42 commits into from
Nov 30, 2023

Conversation

goosewobbler
Copy link
Member

@goosewobbler goosewobbler commented Nov 8, 2023

TODO:

  • Use @vitest/spy
  • Update E2E tests for mock
  • Add E2Es for mockReturnValue
  • Unit tests - execute
  • Unit tests - removeMocks
  • Improve types
  • Fix linting

@goosewobbler goosewobbler added the WIP Work in progress label Nov 8, 2023
@christian-bromann
Copy link
Contributor

@goosewobbler feel free to adjust the test coverage if necessary, we can always increase it later as well.

@goosewobbler
Copy link
Member Author

goosewobbler commented Nov 28, 2023

I think this is good for review, the documentation / API deprecation updates can be done on the main PR and we can follow up with more units.

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Great work, I have one suggestion on the API design

describe('setMock', () => {
it('should mock an electron API function', async () => {
const dialog = await browser.electron.mock('dialog');
await dialog.setMock('showOpenDialog');
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes more sense to just call

const mockedShowOpenDialog = await browser.electron.mock('dialog', 'showOpenDialog')

instead of doing this. It would more align to vitest and jest APIs so user are more familiar with what they need to do.

Is there any advantage of an additional getMock / setMock call?

Copy link
Contributor

Choose a reason for hiding this comment

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

You would call unmock via:

browser.electron.unmock('dialog', 'showOpenDialog')

see https://vitest.dev/api/vi.html#vi-unmock

Copy link
Member Author

@goosewobbler goosewobbler Nov 28, 2023

Choose a reason for hiding this comment

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

I'll see what I can do here, it does make sense to have a single-liner for mocking (and unmocking) an API function but there is a reason I did it this way -- I was running into trouble getting the mock value to update after being called - because it needs to get the calls from the inner mock in the electron main process and re-apply them. I messed around with getters for a while and some other approaches but wasn't able to make the following work:

const mockedShowOpenDialog = await browser.electron.mock('dialog', 'showOpenDialog');
await browser.electron.execute(
  async (electron) =>
    await electron.dialog.showOpenDialog({
      title: 'my dialog',
      properties: ['openFile', 'openDirectory'],
    }),
);

expect(mockedShowOpenDialog).toHaveBeenCalledTimes(1);
expect(mockedShowOpenDialog).toHaveBeenCalledWith({
  title: 'my dialog',
  properties: ['openFile', 'openDirectory'],
});

So, we may be able to condense setMock into mock but a separate call to update the outer mock may well be unavoidable (perhaps e.g. mockedShowOpenDialog.update()), unless there's another way of doing it that I'm not aware of...

Copy link
Member Author

Choose a reason for hiding this comment

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

getting there...

@christian-bromann
Copy link
Contributor

@goosewobbler I wonder if there is a way to have a bidirectional messaging between service and injected Electron code to have these mocks autoupdate, wdyt?

@goosewobbler
Copy link
Member Author

@christian-bromann makes sense that could work, I'll merge this to the original PR and work on it there.

@goosewobbler goosewobbler merged commit b13b0d1 into cb/better-mock Nov 30, 2023
4 checks passed
@goosewobbler goosewobbler deleted the sm/better-mock-updates branch November 30, 2023 12:28
@goosewobbler goosewobbler removed the WIP Work in progress label Nov 30, 2023
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