Skip to content

test(chat): add e2e tests for delete all chats button visibility #1113

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.

Sign up for GitHub

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

Open
fbricon wants to merge 2 commits into
base: main
Choose a base branch
from
+97 −17
Open

test(chat): add e2e tests for delete all chats button visibility #1113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39d7ab9
refactor(chat): add explicit type annotations to ChatPage sendMessage…
fbricon Mar 18, 2026
1609dd1
test(chat): add e2e tests for delete all chats button visibility
fbricon Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion tests/playwright/src/model/pages/chat-page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ export class ChatPage extends BasePage {
}
}

async sendMessage(message: string, { waitForMessage = true, timeout = TIMEOUTS.SHORT } = {}): Promise<void> {
async sendMessage(
message: string,
{ waitForMessage = true, timeout = TIMEOUTS.SHORT }: { waitForMessage?: boolean; timeout?: number } = {},
): Promise<void> {
await this.messageField.fill(message);
await expect(this.messageField).toHaveValue(message);
await expect(this.sendButton).toBeEnabled();
Expand Down Expand Up @@ -210,6 +213,24 @@ export class ChatPage extends BasePage {
return count;
}

async getChatModelIndices(): Promise<number[]> {
Copy link
Contributor

@serbangeorge-m serbangeorge-m Mar 19, 2026

Choose a reason for hiding this comment

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

I do not understand why we need this. What issue is trying to solve? we have already a method handling the dropdown used in multiple tests

Copy link
Contributor Author

@fbricon fbricon Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

because on my Ollama machine, I have a bunch of embedding models, that do not support chat and fail when called. Of course this doesn't happen on the CI, but I need this to be able to run it locally. This wouldn't be an issue if model selection was deterministic. If a given set of models is a prerequisite for running the tests, then we could select those and only those in the tests

Copy link
Contributor

@serbangeorge-m serbangeorge-m Mar 19, 2026

Choose a reason for hiding this comment

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

the tests are design to detect ollama models only when OLLAMA_ENABLED is set true

Copy link
Contributor Author

@fbricon fbricon Mar 19, 2026

Choose a reason for hiding this comment

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

yes I run them locally with OLLAMA_ENABLED=true pnpm run test:e2e --grep "CHAT-", and my embedding model is sometimes picked up

Copy link
Contributor

@serbangeorge-m serbangeorge-m Mar 19, 2026

Choose a reason for hiding this comment

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

This is a local env issue, not a test issue. The tests are designed for CI where only the correct model(granite3.2:2b) is provisioned

Copy link
Contributor Author

@fbricon fbricon Mar 19, 2026

Choose a reason for hiding this comment

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

code like this shows the tests adapt to the environment they're running on:

kortex/tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts

Lines 107 to 112 in b695a9f

const modelCount = await chatPage.getAvailableModelsCount();
if (modelCount < 2) {
test.skip(true, 'Skipping test: Less than 2 models available');
return;
}

If we can't run the e2e tests locally, then there's no incentive to write them, as the feedback loop waiting for CI to run is way too long.

Copy link
Contributor

@benoitf benoitf Mar 19, 2026

Choose a reason for hiding this comment

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

@serbangeorge-m I think it would be great if tests, especially smoke tests, can be more robust when running locally.

We're separating configuration files for testing but here it's more about the ollama instance. From the test POV I would say we don't care if there are already existing models or not, we just want to be sure it's using our models

I know that for containers, for example, sometimes we don't care on all containers that we might have already on a podman instance, we check that if we pull an image, this image is there and that if we start the container, it's there.

so what would you recommend to make the test more robust on running within an existing ollama instance (which of courses can contains things)

Copy link
Contributor

@serbangeorge-m serbangeorge-m Mar 19, 2026

Choose a reason for hiding this comment

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

@benoitf The tests are running ok locally. Rather than tweaking the tests to skip embedding models(which is not a problem at all in the test infrastructure) it would be better to fix this at the product level by filtering the model dropdown based on capabilities. Chat interface should only show models that actually support chat, no?

Copy link
Contributor Author

@fbricon fbricon Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally yes, but there's no surefire way to do it, except querying all models one by one, to see if they maybe contain a chat template, so it's gonna slow things down for sure. Even the Ollama client doesn't do it.

Screenshot 2026-03-19 at 17 36 21

Checking for embed in the model name is a heuristic good enough for tests, but I wouldn't trust it for user facing selection.

We can certainly look into it but I think this goes beyond this PR

Copy link
Contributor

@serbangeorge-m serbangeorge-m Mar 19, 2026

Choose a reason for hiding this comment

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

@fbricon I am ok with the embed filtering in this case but we should really avoid future test adaptations for different custom local environments. Also I think it would be better to return the model names instead of indices to avoid a potential race condition between the dropdown interactions, could you add a selectModelByName() method and use that instead?

await this.modelDropdownSelector.click();
await expect(this.modelDropdownContent).toBeVisible();
try {
const count = await this.modelMenuItems.count();
const indices: number[] = [];
for (let i = 0; i < count; i++) {
const text = await this.modelMenuItems.nth(i).textContent();
if (text && !text.toLowerCase().includes('embed')) {
indices.push(i);
}
}
return indices;
} finally {
await this.page.keyboard.press('Escape');
}
}

async selectModelByIndex(index: number): Promise<void> {
await this.modelDropdownSelector.click();
await expect(this.modelDropdownContent).toBeVisible();
Expand Down
91 changes: 75 additions & 16 deletions tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ test.describe

await chatPage.ensureChatSidebarVisible();
let messageCount = await chatPage.getChatHistoryCount();
if (messageCount > 0) {
await expect(chatPage.deleteAllChatsButton).toBeVisible();
await chatPage.deleteAllChatHistoryItems();
await chatPage.verifyChatHistoryEmpty();
messageCount = 0;
}

const chatSessions = [
{ message: 'What is Kubernetes?', expectedIndex: 1 },
Expand All @@ -80,7 +86,24 @@ test.describe

test('[CHAT-04] Delete single chat item and then delete all remaining items', async ({ chatPage }) => {
await chatPage.ensureChatSidebarVisible();
const initialCount = await chatPage.getChatHistoryCount();
let initialCount = await chatPage.getChatHistoryCount();

// Create at least 2 chats if none exist
if (initialCount < 2) {
for (let i = initialCount; i < 2; i++) {
await chatPage.clickNewChat();
const message = `Test message ${i + 1}`;
await chatPage.sendMessage(message, { timeout: 100 });
await chatPage.verifyConversationMessage(message);
}
await expect
.poll(async () => await chatPage.getChatHistoryCount(), { timeout: TIMEOUTS.MODEL_RESPONSE })
.toBeGreaterThanOrEqual(2);
initialCount = await chatPage.getChatHistoryCount();
}

// Verify delete all button is visible when there are chats
await expect(chatPage.deleteAllChatsButton).toBeVisible();

await chatPage.deleteChatHistoryItemByIndex(0);
const expectedCountAfterSingleDelete = initialCount - 1;
Expand All @@ -91,6 +114,8 @@ test.describe
await chatPage.clickChatHistoryItemByIndex(0);
// Verify we're viewing the conversation (no suggested messages)
await expect(chatPage.suggestedMessagesGrid).not.toBeVisible();
// Delete all button should still be visible
await expect(chatPage.deleteAllChatsButton).toBeVisible();
}

await chatPage.deleteAllChatHistoryItems();
Expand All @@ -100,27 +125,32 @@ test.describe
await chatPage.verifySuggestedMessagesVisible();
await expect(chatPage.conversationMessages).toHaveCount(0);

// Verify delete all button is no longer visible when there are no chats
await expect(chatPage.deleteAllChatsButton).not.toBeVisible();

await chatPage.ensureNotificationsAreNotVisible();
});

test('[CHAT-05] Switch between all available models and verify each selection', async ({ chatPage }) => {
const modelCount = await chatPage.getAvailableModelsCount();
const chatModelIndices = await chatPage.getChatModelIndices();

if (modelCount < 2) {
test.skip(true, 'Skipping test: Less than 2 models available');
if (chatModelIndices.length < 2) {
test.skip(true, 'Skipping test: Less than 2 chat models available');
return;
}

const maxModelsToTest = Math.min(modelCount, 3);
const indicesToTest = chatModelIndices.slice(0, 3).reverse();

for (const modelIndex of indicesToTest) {
await chatPage.clickNewChat(); // Select a new chat before selecting a model

for (let modelIndex = maxModelsToTest - 1; modelIndex >= 0; modelIndex--) {
await chatPage.selectModelByIndex(modelIndex);
const selectedModelName = await chatPage.getSelectedModelName();
let selectedModelName = await chatPage.getSelectedModelName();
expect(selectedModelName).toBeTruthy();
// TODO(https://github.com/kortex-hub/kortex/issues/1107): a message containing a colon is not displayed correctly in the conversation history
selectedModelName = selectedModelName.replace(/:/g, '-');

await chatPage.clickNewChat();

const testMessage = `Test message for model: ${selectedModelName}`;
const testMessage = `Test message for model: "${selectedModelName}"`;
await chatPage.sendMessage(testMessage);
await chatPage.verifyConversationMessage(testMessage);
}
Expand All @@ -129,10 +159,10 @@ test.describe
test('[CHAT-06] Change models mid-conversation, verify conversation history is preserved', async ({ chatPage }) => {
test.slow();

const modelCount = await chatPage.getAvailableModelsCount();
const chatModelIndices = await chatPage.getChatModelIndices();

if (modelCount < 2) {
test.skip(true, 'Skipping test: Less than 2 models available');
if (chatModelIndices.length < 2) {
test.skip(true, 'Skipping test: Less than 2 chat models available');
return;
}

Expand All @@ -141,8 +171,8 @@ test.describe
const initialCount = await chatPage.getChatHistoryCount();

const modelSwitches = [
{ modelIndex: 0, message: 'Hello, how are you?' },
{ modelIndex: 1, message: 'Tell me about AI models' },
{ modelIndex: chatModelIndices[0], message: 'Hello, how are you?' },
{ modelIndex: chatModelIndices[1], message: 'Tell me about AI models' },
];

const sentMessages: string[] = [];
Expand Down Expand Up @@ -184,7 +214,7 @@ test.describe

await navigationBar.navigateToChatPage();

await expect(chatPage.toolsSelectionButton).toBeVisible({ timeout: TIMEOUTS.SHORT });
await expect(chatPage.toolsSelectionButton).toBeVisible({ timeout: TIMEOUTS.MODEL_RESPONSE });
await expect(chatPage.configureMcpServersButton).not.toBeVisible();

await chatPage.ensureToolsSidebarVisible();
Expand Down Expand Up @@ -268,6 +298,35 @@ test.describe
await chatPage.verifyStopButtonHidden();
});

test('[CHAT-10] Delete all button remains visible without scrolling', async ({ chatPage }) => {
await chatPage.ensureChatSidebarVisible();

const expectedChats = 15;
const initialCount = await chatPage.getChatHistoryCount();

// Create missing chats to reach the expected count
if (initialCount < expectedChats) {
const chatsToCreate = expectedChats - initialCount;
for (let i = 0; i < chatsToCreate; i++) {
await chatPage.clickNewChat();
await chatPage.sendMessage(`Reply "OK ${i + 1}", nothing else.`, { waitForMessage: false });
}

// Wait for all chats to appear in history
await expect
.poll(async () => await chatPage.getChatHistoryCount(), { timeout: TIMEOUTS.MODEL_RESPONSE })
.toBeGreaterThanOrEqual(expectedChats);
}

// Verify delete all button is visible in viewport without scrolling
await expect(chatPage.deleteAllChatsButton).toBeInViewport();

// Clean up - delete all chats
await chatPage.deleteAllChatHistoryItems();
await chatPage.verifyChatHistoryEmpty();
await chatPage.ensureNotificationsAreNotVisible();
});

test('[CHAT-11] Last used model is remembered when starting a new chat', async ({ chatPage }) => {
const modelCount = await chatPage.getAvailableModelsCount();

Expand Down
Loading