Issue/4637 optional translations act as required #180
Conversation
There was a problem hiding this comment.
Nice to see extensive testing being applied!
My general theme of feedback is about the existing tests and sync/async behaviours, which unfortunately is quite tied to exactly how one holds Storybook...
Another thing that caught my attention was the amount of commit messages - once the PR is ready for check-in, could you rebase the commits to be a bit more self-contained? E.g. commits for prettier can easily be squashed in their respective commits that do the bugfix/add the (regression) test/implement a feature.
In Open Forms we rely on the commit history really hard to debug and understand code, so it's important that we see "the useful" information in a git blame.
| const inputs = editForm.getAllByRole('textbox'); | ||
| for (let input of inputs) { | ||
| await userEvent.type(input, 'manualTranslation'); | ||
| await expect(input).toHaveValue('manualTranslation'); |
There was a problem hiding this comment.
expect should not necessarily be await-ed. It shouldn't break anything, but it is a bit confusing. Typically it should be something like expect(await canvas.findByRole(...)).toBeVisible() to account for some rendering delays etc.
these existing tests are littered with bad examples, so I can hardly blame you for this.
| await userEvent.type(input, 'manualTranslation'); | ||
| await expect(input).toHaveValue('manualTranslation'); | ||
| await userEvent.clear(input); | ||
| await expect(input).toHaveValue(''); |
There was a problem hiding this comment.
| await expect(input).toHaveValue(''); | |
| expect(input).toHaveValue(''); |
| const inputs = editForm.getAllByRole('textbox'); | ||
| for (let input of inputs) { | ||
| await userEvent.type(input, 'manualTranslation'); | ||
| await expect(input).toHaveValue('manualTranslation'); |
There was a problem hiding this comment.
| await expect(input).toHaveValue('manualTranslation'); | |
| expect(input).toHaveValue('manualTranslation'); |
| await userEvent.click(canvas.getByRole('tab', {name: 'Translations'})); | ||
|
|
||
| // Check that none of the inputs have a Required error message | ||
| await expect(await editForm.queryByText('Required')).toBeNull(); |
There was a problem hiding this comment.
| await expect(await editForm.queryByText('Required')).toBeNull(); | |
| expect(await editForm.queryByText('Required')).toBeNull(); |
| expect(await canvas.findByText('EN')).toBeVisible(); | ||
| expect(await canvas.findByText('Error code')).toBeVisible(); | ||
|
|
||
| await waitFor(async () => { |
There was a problem hiding this comment.
it's a bit strange that here the waitFor is needed, since the await canvas.findByLabelText(...) is already async and uses waitFor under the hood 🤔
I'd like to better understand if/what is going wrong in these interaction tests - often an actual root cause can be found.
There was a problem hiding this comment.
Without the waitFor they keep failing locally..
Maybe this is again one of those situations that only fail locally, and that the CI will work fine..
| await waitFor(async () => { | ||
| await expect( | ||
| await canvas.findByText('The option label is a required field.') | ||
| ).toBeVisible(); | ||
| await expect( | ||
| await canvas.findByText('The option value is a required field.') | ||
| ).toBeVisible(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
these often fail locally in storybook when you don't have focus on the browser window. could you check if they're really necessary? I can't really remember these tests failing in CI, while the file size validation tests are notable sources of flakiness
There was a problem hiding this comment.
Its a bit weird/scary that some tests (only) fail locally.. I'll check them out and remove the await's and waitFor's that aren't necessary
There was a problem hiding this comment.
yeah... time for my weekly rant about the state of testing in JS-land I guess 😅
robinmolen
force-pushed
the
issue/4637-optional-translations-act-as-required
branch
4 times, most recently
from
6623aa9 to
883f5a6
Compare
…, select and selectboxes component tests
robinmolen
force-pushed
the
issue/4637-optional-translations-act-as-required
branch
from
883f5a6 to
3a98100
Compare
robinmolen
deleted the
issue/4637-optional-translations-act-as-required
branch
The translations of the option values where required because of a small issue in the zog validation configuration. This is now fixed
I've also added some tests to the componentConfigurations story, and to the stories of the individual components. Each story now has tests for the translation inputs. Ensuring that all translation inputs can be targeted, filled in, cleared and result in no errors.