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

FTR for playground #186246

Merged

Conversation

Samiul-TheSoccerFan
Copy link
Contributor

@Samiul-TheSoccerFan Samiul-TheSoccerFan commented Jun 14, 2024

Summary

This PR contains high-priority FTR tests for the playground in ESS.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@Samiul-TheSoccerFan Samiul-TheSoccerFan added v8.15.0 release_note:skip Skip the PR/issue when compiling release notes Team:Search labels Jun 14, 2024
await pageObjects.searchPlayground.PlaygroundStartChatPage.expectSelectIndex(indexName);
it('creates a connector successfully', async () => {
await pageObjects.searchPlayground.PlaygroundStartChatPage.expectOpenConnectorPagePlayground();
await pageObjects.searchPlayground.PlaygroundStartChatPage.createOpenAIConnector(proxy);
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to create an api function for creating connector as we don't need to test third-party library. Because if they changed name of field it will broke our test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although a different team maintains it, the main reason to add this step in the test is to verify the flyout opens and the user can create a Gen AI connector and use it immediately.

Copy link
Contributor

@yansavitski yansavitski Jun 21, 2024

Choose a reason for hiding this comment

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

Yeah I agree that we need to test how flyout is opening, because this is related to our code, but testing the flyout itself seems like an antipattern “Third-Party Code Testing.” Which can lead to:

  • Unnecessary Maintenance: tests might fail if the third-party library updates or changes, leading to maintenance work unrelated to your actual code.
  • Redundancy: Most third-party libraries come with their own tests. Testing them again in integration suite duplicates effort without additional value.
  • False Confidence: Passing tests might give a false sense of security about the library’s behavior under all conditions, which isn’t practical to ensure comprehensively.

@Samiul-TheSoccerFan Samiul-TheSoccerFan force-pushed the addLLMProxyForPlayground branch from 088617d to e389932 Compare June 21, 2024 17:57
@Samiul-TheSoccerFan Samiul-TheSoccerFan marked this pull request as ready for review June 21, 2024 18:00
@Samiul-TheSoccerFan Samiul-TheSoccerFan requested a review from a team as a code owner June 21, 2024 18:00
@Samiul-TheSoccerFan Samiul-TheSoccerFan changed the title Add llm proxy for playground ESS FTR for playground Jun 21, 2024
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6365

[✅] x-pack/test/functional/apps/search_playground/config.ts: 25/25 tests passed.

see run history

Comment on lines 143 to 145
const [conversationSimulator] = await Promise.all([
conversationInterceptor.waitForIntercept(),
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need here promise.all. guess just forgot to remove it after reducing titleInterceptor

Copy link
Contributor

@yansavitski yansavitski left a comment

Choose a reason for hiding this comment

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

Really great job with making llmProxy works 😎

@Samiul-TheSoccerFan
Copy link
Contributor Author

@elasticmachine merge upstream

@Samiul-TheSoccerFan
Copy link
Contributor Author

@elasticmachine merge upstream

@Samiul-TheSoccerFan Samiul-TheSoccerFan changed the title ESS FTR for playground FTR for playground Jun 25, 2024
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6393

[✅] x-pack/test/functional/apps/search_playground/config.ts: 25/25 tests passed.

see run history

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6394

[✅] x-pack/test_serverless/functional/test_suites/search/config.ts: 25/25 tests passed.

see run history

@Samiul-TheSoccerFan Samiul-TheSoccerFan force-pushed the addLLMProxyForPlayground branch from 155b6d0 to c49db73 Compare June 26, 2024 21:29
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #12 / Case View Page files tab should render the custom fields correctly

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
searchPlayground 167.7KB 167.9KB +219.0B

History

  • 💚 Build #217705 succeeded 155b6d008c565981897de032f5e03b82ea0c44b3
  • 💔 Build #217550 failed d030432f129e74025c41bb1cdfc053d9c1d5736a
  • 💔 Build #217541 failed a92f0cb4c6f6be740155e7d9a4a88659ed26f5be

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Samiul-TheSoccerFan Samiul-TheSoccerFan merged commit 5324dc2 into elastic:main Jun 27, 2024
18 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Search v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants