-
Notifications
You must be signed in to change notification settings - Fork 233
chore(compass-e2e-tests): add assistant end to end tests COMPASS-9748 #7429
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive end-to-end tests for the MongoDB Assistant feature in Compass. The tests cover various scenarios including AI feature opt-in flow, message sending, drawer interactions, and entry points from explain plans and error messages.
- Adds complete e2e test suite for Assistant functionality with 593 lines of test code
- Implements mock assistant service to simulate AI responses with streaming support
- Adds necessary test selectors and data-testid attributes for UI elements
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/compass-generative-ai/src/components/ai-optin-modal.tsx | Adds data-testid for testing the AI opt-in modal |
packages/compass-e2e-tests/tests/assistant.test.ts | Complete test suite covering assistant drawer, opt-in flow, messaging, and entry points |
packages/compass-e2e-tests/helpers/selectors.ts | Adds selectors for assistant UI elements and AI settings |
packages/compass-e2e-tests/helpers/assistant-service.ts | Mock service implementation for simulating assistant API responses |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Assigned |
|
||
describe('drawer visibility', function () { | ||
it('shows the assistant drawer button when AI features are enabled', async function () { | ||
await setAIFeatures(browser, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so hypothetically this function should work and it does in one direction but after opting in, going back to opt-out isn't possible. Basically as a result the tests are order-dependent which isn't great. I think it's because of some of our logic and possibly related to the more general issue with i.e. feature flags we were seeing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lerouxb suggested
compass = await init(this.test?.fullTitle(), { firstRun: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that doesn't work then I guess we can also add a hidden way to opt-out again.
Either we can just tweak settings so it can be overridden or we can add some custom event like we did for selecting a file:
compass/packages/compass-import-export/src/components/export-modal.tsx
Lines 224 to 240 in b9a6fde
useEffect(() => { | |
if (isOpen) { | |
// For e2e testing we can't set the value of a file output | |
// for security reasons, so we listen to a dom event that sets it. | |
// https://github.com/electron-userland/spectron/issues/23 | |
document.addEventListener( | |
'selectExportFileName', | |
onSelectExportFileNameEvent | |
); | |
return () => { | |
document.removeEventListener( | |
'selectExportFileName', | |
onSelectExportFileNameEvent | |
); | |
}; | |
} | |
}, [isOpen, onSelectExportFileNameEvent]); |
or what we did with test:show-connect-window
'test:show-connect-window': () => void showConnectWindow(compassApp), |
or what we did to get things like the logs:
compass/packages/compass-e2e-tests/helpers/compass.ts
Lines 226 to 229 in b9a6fde
ipcRenderer.invoke('compass:logPath'), | |
ipcRenderer.invoke('compass:userDataPath'), | |
ipcRenderer.invoke('compass:appName'), | |
ipcRenderer.invoke('compass:mainProcessPid'), |
I don't know which one is the right way I'm just saying there are probably options if we need them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or see browser.getFeature() and browser.setFeature() For example
compass/packages/compass-web/src/preferences.tsx
Lines 14 to 51 in b9a6fde
const kSandboxUpdateFn = Symbol.for('@compass-web-sandbox-update-preferences'); | |
/** | |
* Only used in the sandbox to provide a way to update preferences. | |
* @internal | |
*/ | |
export const SandboxPreferencesUpdateProvider = ({ | |
children, | |
}: { | |
children: React.ReactNode; | |
}) => { | |
const [updateTrigger] = useState<SandboxPreferencesUpdateTrigger>(() => { | |
return ( | |
updatePreferencesFn: ( | |
preferences: Partial<AllPreferences> | |
) => Promise<AllPreferences> | |
) => { | |
// eslint-disable-next-line no-console | |
console.info( | |
`[compass-web sandbox] call window[Symbol.for('@compass-web-sandbox-update-preferences')]({}) to dynamically update preferences` | |
); | |
(globalThis as any)[kSandboxUpdateFn] = ( | |
preferences: Partial<AllPreferences> | |
) => { | |
return updatePreferencesFn(preferences); | |
}; | |
return () => { | |
delete (globalThis as any)[kSandboxUpdateFn]; | |
}; | |
}; | |
}); | |
return ( | |
<SandboxPreferencesUpdateTriggerContext.Provider value={updateTrigger}> | |
{children} | |
</SandboxPreferencesUpdateTriggerContext.Provider> | |
); | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser.setFeature
is what this one uses behind the scenes, doesn't seem to fully do the trick so will just do the hard reset
} | ||
} | ||
|
||
async function getDisplayedMessages(browser: CompassBrowser) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally defined these functions in hooks but it was making the code too dirty so I moved them out here and made it pass browser instead... I imagine if chat becomes popular enough we can extend browser itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine - I usually add things locally and then only factor them out once necessary. I think that's a good way to work.
]); | ||
}); | ||
|
||
it('can copy assistant message to clipboard', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that for now this one is not going to work for the web tests. (we also run the sandbox in the browser if I remember correctly and there might be ones that hit DE - I'm a bit out of the loop).
|
||
mockAssistantServer.setResponse({ | ||
status: 200, | ||
body: 'You should create an index.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
await browser.setFeature('optInGenAIFeatures', enabled); | ||
|
||
// Wait for the IPC to be processed | ||
await browser.pause(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of pause is kinda dodgy and shouldn't be necessary because anything depending on the result should/would be waiting for whatever it is expecting to happen anyway.
const drawerButton = browser.$(Selectors.AssistantDrawerButton); | ||
await drawerButton.waitForDisplayed(); | ||
await drawerButton.waitForClickable(); | ||
await drawerButton.click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI in theory await browser.clickVisible(Selectors.AssistantDrawerButton);
does all this and more.
Would have been better if we added it as a custom element command rather than a browser command, but it is there.
|
||
async function getDisplayedMessages(browser: CompassBrowser) { | ||
// Wait for the messages container to be visible | ||
const chatMessages = browser.$(Selectors.AssistantChatMessages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor style nit (and I don't think we're very consistent about it), but if you're not going to use the chatMessages var again you can also just chain like await browser.$(Selectors.AssistantChatMessages).waitForDisplayed();
const explainModal = browser.$(Selectors.AggregationExplainModal); | ||
await explainModal.waitForDisplayed(); | ||
|
||
// Wait for the explain plan to be ready (loader should disappear) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally remove this whole block because the loader can appear/disappear instantly or it can happen within one of the intervals that any wait command polls. ie. it is going to be flaky. Plus it also suffers from the problem that the loader starts off not being there.
You can just wait for the interpret button to be there and click it - it should be the same result anyway.
await explainModal.waitForDisplayed({ reverse: true }); | ||
|
||
// The assistant drawer should open | ||
const assistantDrawer = browser.$(Selectors.SideDrawer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not rather wait for the assistant drawer section specifically? In case over time we also end up with the Data Modelling section in there.
} | ||
|
||
async function useErrorViewEntryPoint(browser: CompassBrowser) { | ||
const connectionToastErrorDebugButton = browser.$( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I mentioned this above too but I really think you can just await browser.clickVisible(Selectors.ConnectionToastErrorDebugButton);
which does most of this. I suppose you can wait for the toast to disappear afterwards if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is nitpicking, btw. What you have here is fine.)
sendChunk(); | ||
} | ||
|
||
export async function startMockAssistantServer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So.. Usually E2E tests are purposefully integration tests, ie. we try not to mock or stub anything if at all possible. But I do get that the assistant is an expensive feature to be running many times on every PR and it is also particularly non-deterministic.
So I think in this case it is OK, just something to keep in mind going forward.
Adds e2e tests to the core assistant features.