-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cases] Fix flaky tests in the create case form #145211
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
4a4b9e5
to
bd71310
Compare
bd71310
to
6a0e541
Compare
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.
Looks good, just left a few nits
for (let i = 0; i < sampleTags.length; i++) { | ||
const tagsInput = await within(caseTags).findByTestId('comboBoxInput'); | ||
userEvent.type(tagsInput, `${sampleTags[i]}{enter}`); | ||
for (let i = 0; i < sampleTags.length; i++) { |
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.
nit: do we need a counter in the for-loop? Could we switch this to a for-of-loop:
for (const tag of sampleTags) {
...
}
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.
Oh sorry, I didn't look at the previous version 🤦♂️ that's the way it was before. Ignore me.
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.
No worries! It is a good opportunity to improve it
describe.skip('Create case', () => { | ||
const waitForFormToRender = async (renderResult: RenderResult) => { | ||
await waitFor(() => { | ||
expect(renderResult.getByTestId('caseTitle')).toBeTruthy(); |
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.
nit: It sounds like the general advice is to start using screen
. Using screen would also avoid having to pass in the renderResult
parameter too
https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-screen
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.
You are right!
tags: [], | ||
}; | ||
|
||
const fillFormReactTestingLib = async (renderResult: RenderResult, withTags = 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.
super nit: I try not to have functions take booleans because it makes it difficult to know what the parameter means when you just look at the function call like this:
fillFormReactTestingLib(renderResult, true);
One option would be to pass an object so it's descriptive (I'm not sure how annoying that'd be to have to update throughout this whole file though)
const fillFormReactTestingLib = async ({renderResult, withTags = false}: {renderResult: RenderResult, withTags: boolean})
Or another option would be to create a new function that wraps this one with descriptive name indicating that it is filling the form with tags:
const fillFormIncludeTags = async () => {
await fillFormReactTestingLib(renderResult, 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.
I was lazy, you are right!
it('does not submits the title when the length is longer than 64 characters', async () => { | ||
const longTitle = | ||
'This is a title that should not be saved as it is longer than 64 characters.{enter}'; | ||
it('does not submits the title when the length is longer than 160 characters', async () => { |
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.
🙌
const longTitle = | ||
'This is a title that should not be saved as it is longer than 64 characters.{enter}'; | ||
it('does not submits the title when the length is longer than 160 characters', async () => { | ||
const longTitle = `${'a'.repeat(161)}`; |
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.
nit:
const longTitle = `${'a'.repeat(161)}`; | |
const longTitle = 'a'.repeat(161); |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @cnasikas |
## Summary This PR fixes a number of issues with the same error: `TestingLibraryElementError: Unable to find an element by: [data-test-subj="caseTitle"]`. The PR: - Clears unnecessary `act` - Wait for the form to render before trying to fill the form - Wait for the component to update all states to eliminate warnings - Fill tags when necessary to improve tests execution time - Replace `userEvent.type` with `userEvent.aste` when possible to improve execution time - Add `skipPointerEventsCheck: true` when necessary I run the test file 100 times locally without any errors. Fixes: elastic#142287, elastic#142288, elastic#142285, elastic#142286, elastic#142284, elastic#142283, elastic#142282, elastic#142281, elastic#143407, elastic#143406, elastic#143405, elastic#143408, elastic#143403 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 6936644)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
) # Backport This will backport the following commits from `main` to `8.6`: - [[Cases] Fix flaky tests in the create case form (#145211)](#145211) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Christos Nasikas","email":"[email protected]"},"sourceCommit":{"committedDate":"2022-11-16T20:13:58Z","message":"[Cases] Fix flaky tests in the create case form (#145211)\n\n## Summary\r\n\r\nThis PR fixes a number of issues with the same error:\r\n`TestingLibraryElementError: Unable to find an element by:\r\n[data-test-subj=\"caseTitle\"]`. The PR:\r\n\r\n- Clears unnecessary `act`\r\n- Wait for the form to render before trying to fill the form\r\n- Wait for the component to update all states to eliminate warnings\r\n- Fill tags when necessary to improve tests execution time\r\n- Replace `userEvent.type` with `userEvent.aste` when possible to\r\nimprove execution time\r\n- Add `skipPointerEventsCheck: true` when necessary\r\n\r\nI run the test file 100 times locally without any errors.\r\n\r\nFixes: https://github.com/elastic/kibana/issues/142287,\r\nhttps://github.com/elastic/kibana/issues/142288,\r\nhttps://github.com/elastic/kibana/issues/142285,\r\nhttps://github.com/elastic/kibana/issues/142286,\r\nhttps://github.com/elastic/kibana/issues/142284,\r\nhttps://github.com/elastic/kibana/issues/142283,\r\nhttps://github.com/elastic/kibana/issues/142282,\r\nhttps://github.com/elastic/kibana/issues/142281,\r\nhttps://github.com/elastic/kibana/issues/143407,\r\nhttps://github.com/elastic/kibana/issues/143406,\r\nhttps://github.com/elastic/kibana/issues/143405,\r\nhttps://github.com/elastic/kibana/issues/143408,\r\nhttps://github.com/elastic/kibana/issues/143403\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6936644ab05362510d91d1487202a6ee5dbc478f","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","Feature:Cases","v8.6.0","v8.7.0"],"number":145211,"url":"https://github.com/elastic/kibana/pull/145211","mergeCommit":{"message":"[Cases] Fix flaky tests in the create case form (#145211)\n\n## Summary\r\n\r\nThis PR fixes a number of issues with the same error:\r\n`TestingLibraryElementError: Unable to find an element by:\r\n[data-test-subj=\"caseTitle\"]`. The PR:\r\n\r\n- Clears unnecessary `act`\r\n- Wait for the form to render before trying to fill the form\r\n- Wait for the component to update all states to eliminate warnings\r\n- Fill tags when necessary to improve tests execution time\r\n- Replace `userEvent.type` with `userEvent.aste` when possible to\r\nimprove execution time\r\n- Add `skipPointerEventsCheck: true` when necessary\r\n\r\nI run the test file 100 times locally without any errors.\r\n\r\nFixes: https://github.com/elastic/kibana/issues/142287,\r\nhttps://github.com/elastic/kibana/issues/142288,\r\nhttps://github.com/elastic/kibana/issues/142285,\r\nhttps://github.com/elastic/kibana/issues/142286,\r\nhttps://github.com/elastic/kibana/issues/142284,\r\nhttps://github.com/elastic/kibana/issues/142283,\r\nhttps://github.com/elastic/kibana/issues/142282,\r\nhttps://github.com/elastic/kibana/issues/142281,\r\nhttps://github.com/elastic/kibana/issues/143407,\r\nhttps://github.com/elastic/kibana/issues/143406,\r\nhttps://github.com/elastic/kibana/issues/143405,\r\nhttps://github.com/elastic/kibana/issues/143408,\r\nhttps://github.com/elastic/kibana/issues/143403\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6936644ab05362510d91d1487202a6ee5dbc478f"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145211","number":145211,"mergeCommit":{"message":"[Cases] Fix flaky tests in the create case form (#145211)\n\n## Summary\r\n\r\nThis PR fixes a number of issues with the same error:\r\n`TestingLibraryElementError: Unable to find an element by:\r\n[data-test-subj=\"caseTitle\"]`. The PR:\r\n\r\n- Clears unnecessary `act`\r\n- Wait for the form to render before trying to fill the form\r\n- Wait for the component to update all states to eliminate warnings\r\n- Fill tags when necessary to improve tests execution time\r\n- Replace `userEvent.type` with `userEvent.aste` when possible to\r\nimprove execution time\r\n- Add `skipPointerEventsCheck: true` when necessary\r\n\r\nI run the test file 100 times locally without any errors.\r\n\r\nFixes: https://github.com/elastic/kibana/issues/142287,\r\nhttps://github.com/elastic/kibana/issues/142288,\r\nhttps://github.com/elastic/kibana/issues/142285,\r\nhttps://github.com/elastic/kibana/issues/142286,\r\nhttps://github.com/elastic/kibana/issues/142284,\r\nhttps://github.com/elastic/kibana/issues/142283,\r\nhttps://github.com/elastic/kibana/issues/142282,\r\nhttps://github.com/elastic/kibana/issues/142281,\r\nhttps://github.com/elastic/kibana/issues/143407,\r\nhttps://github.com/elastic/kibana/issues/143406,\r\nhttps://github.com/elastic/kibana/issues/143405,\r\nhttps://github.com/elastic/kibana/issues/143408,\r\nhttps://github.com/elastic/kibana/issues/143403\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6936644ab05362510d91d1487202a6ee5dbc478f"}}]}] BACKPORT--> Co-authored-by: Christos Nasikas <[email protected]>
Summary
This PR fixes a number of issues with the same error:
TestingLibraryElementError: Unable to find an element by: [data-test-subj="caseTitle"]
. The PR:act
userEvent.type
withuserEvent.aste
when possible to improve execution timeskipPointerEventsCheck: true
when necessaryI run the test file 100 times locally without any errors.
Fixes: #142287, #142288, #142285, #142286, #142284, #142283, #142282, #142281, #143407, #143406, #143405, #143408, #143403
Checklist
Delete any items that are not applicable to this PR.
For maintainers