Skip to content

Conversation

marcjansen
Copy link
Member

@marcjansen marcjansen commented Sep 30, 2025

Description

This PR suggests to add several unit tests to increase the coverage percentage.

Two bugs wer uncovered, these are adressed in c2b5824, this should be carefully checked by any reviewers.

Related issues or pull requests

None.

Pull request type

  • Bugfix
  • Feature
  • Dependency updates
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe): tests

Do you introduce a breaking change?

  • Yes
  • No

Checklist

  • I understand and agree that the changes in this PR will be licensed under the
    BSD-2-Clause.
  • I have followed the guidelines for contributing.
  • The proposed change fits to the content of the Code of Conduct.
  • I have added or updated tests and documentation, and the test suite passes (run npm run check locally).
  • I have added a screenshot/screencast to illustrate the visual output of my update.

@marcjansen marcjansen force-pushed the increase-test-coverage branch from c1cc734 to 10815d6 Compare September 30, 2025 15:04
@marcjansen marcjansen marked this pull request as ready for review September 30, 2025 15:13
);
const btn = container.querySelector('.change-bg-btn');
await act(async () => {
btn && fireEvent.click(btn);
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is only because of the querySelector possibly returning null? To satisfy the typechecking?

If the btn does not exist in the document, the event will not be fired and the error might be hard to debug.

maybe something like this works as well:

const btn = await waitFor(() => container.querySelector('.btn'));

await act(async () => {
  fireEvent.click(btn);
});

?

<div className="layer-cards">
{
layers.map(layer => (
layers.filter(backgroundLayerFilter).map(layer => (
Copy link
Member

Choose a reason for hiding this comment

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

nice catches!

it('calls onCancel when cancel is clicked', () => {
const onCancel = jest.fn();
render(<FeatureLabelModal feature={feature} onOk={jest.fn()} onCancel={onCancel} />);
// AntD Modal renders a button with aria-label 'Close' for cancel
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// AntD Modal renders a button with aria-label 'Close' for cancel

aria-label 'Close' is not used so the comment is not needed?

fireEvent.change(input, { target: { value: '1234567890' } });
const okBtn = screen.getByRole('button', { name: /ok/i });
fireEvent.click(okBtn);
// the implementation is a bit surprising, but this is it rght now
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@simonseyock simonseyock left a comment

Choose a reason for hiding this comment

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

Nice! And some good catches.

@sdobbert
Copy link
Contributor

sdobbert commented Oct 1, 2025

Some tests might be dublicated? #4379

@marcjansen
Copy link
Member Author

Some tests might be dublicated? #4379

@sdobbert Yes, I am i favour of mergin yours first and then I'll update this PR.

@simonseyock thanks for the Feedback, will adjust when I update this PR.

@marcjansen marcjansen mentioned this pull request Oct 1, 2025
15 tasks
@sdobbert sdobbert mentioned this pull request Oct 2, 2025
15 tasks
@sdobbert
Copy link
Contributor

sdobbert commented Oct 2, 2025

Some tests might be dublicated? #4379

@sdobbert Yes, I am i favour of mergin yours first and then I'll update this PR.

@simonseyock thanks for the Feedback, will adjust when I update this PR.

I opened a new PR (#4399) merging some of these tests and changes into the existing testsuites from #4379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants