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

feat: add test cases for assistant conversation history #1010

Conversation

wanglam
Copy link
Collaborator

@wanglam wanglam commented Jan 24, 2024

Description

Add tests for conversation history, here is the local test video:

conversation_history_spec.js.mp4

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@wanglam wanglam marked this pull request as ready for review January 24, 2024 09:45
Comment on lines 68 to 72
conversations.forEach(({ conversationId }) => {
cy.get(
`div[data-test-subj="chatHistoryItem-${conversationId}"]`
).should('exist');
});
Copy link
Member

Choose a reason for hiding this comment

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

conversations here should be an empty array, is that intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, this case should be moved under history item operations.

`div[data-test-subj="chatHistoryItem-${conversationId}"]`
).should('exist');
});
cy.contains('What are the indices in my cluster?');
Copy link
Member

Choose a reason for hiding this comment

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

This is a conversation from basic_spec.js right? Maybe we should not test that in this spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is for waiting assistant has been loaded successfully. I think we can keep it.

Copy link
Member

Choose a reason for hiding this comment

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

Typo: conversation_history_spec.js

// Set welcome screen tracking to false
localStorage.setItem('home:welcome:show', 'false');
// Hide new theme modal
localStorage.setItem('home:newThemeModal:show', 'false');
Copy link
Member

Choose a reason for hiding this comment

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

do we want to get their current value first and then set the back to the original value after these tests are complete in case there are tests specific to these values.

);

// Open chat flyout
cy.get('body')
Copy link
Member

Choose a reason for hiding this comment

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

some of these look like we can add test subjects to the source code so that it is easier to find the values.

Copy link
Collaborator Author

@wanglam wanglam Jan 25, 2024

Choose a reason for hiding this comment

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

This step is for check if flyout already opened before. Since we are visiting a new page. I think we can remove this check logic here. The flyout must be closed by default.

after(() => {
// Clear created conversations in tests
conversations.map(({ conversationId }) =>
cy.deleteConversation(conversationId)
Copy link
Member

Choose a reason for hiding this comment

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

is there a bulk delete API that we can use? are will there be no need to implement that API.

@@ -152,3 +153,14 @@ Cypress.Commands.add('stopDummyServer', () => {
}
});
});

Cypress.Commands.add('sendMessage', (body) =>
Copy link
Member

Choose a reason for hiding this comment

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

will this be too generic? trying to think if there might be a time where we want to avoid a conflict but I believe assistant is the only thing with a message or conversation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will change to sendAssistantMessage.

@ruanyl
Copy link
Member

ruanyl commented Jan 25, 2024

@wanglam wanglam marked this pull request as draft January 26, 2024 06:06
Signed-off-by: Lin Wang <[email protected]>
@wanglam wanglam force-pushed the feat-add-assistant-conversation-history-test branch from 05e373a to 4f6e751 Compare January 27, 2024 05:23
@wanglam wanglam marked this pull request as ready for review January 27, 2024 05:32
@wanglam
Copy link
Collaborator Author

wanglam commented Jan 29, 2024

@wanglam could you check the failed tests in this workflow run? https://github.com/opensearch-project/opensearch-dashboards-functional-test/actions/runs/7638747582/job/20810302393?pr=1010

@ruanyl Added 10s delayed, now the workflow fixed.

@ruanyl ruanyl merged commit b65c5de into opensearch-project:main Feb 2, 2024
38 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 2, 2024
* feat: add test cases for assistant conversation history

Signed-off-by: Lin Wang <[email protected]>

* feat: move created history test below

Signed-off-by: Lin Wang <[email protected]>

* rename coversation_history_spec to conversation_history_spec

Signed-off-by: Lin Wang <[email protected]>

* Address pr comments

Signed-off-by: Lin Wang <[email protected]>

* refactor: move setStorageItem to helpers and change to getElementByTestId

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Lin Wang <[email protected]>
(cherry picked from commit b65c5de)
ruanyl added a commit that referenced this pull request Feb 2, 2024
* feat: add test cases for assistant conversation history

Signed-off-by: Lin Wang <[email protected]>

* feat: move created history test below

Signed-off-by: Lin Wang <[email protected]>

* rename coversation_history_spec to conversation_history_spec

Signed-off-by: Lin Wang <[email protected]>

* Address pr comments

Signed-off-by: Lin Wang <[email protected]>

* refactor: move setStorageItem to helpers and change to getElementByTestId

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Lin Wang <[email protected]>
(cherry picked from commit b65c5de)

Co-authored-by: Lin Wang <[email protected]>
Co-authored-by: Yulong Ruan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants