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[mal]: Map and Label Error and Validation Tests #3621

Merged
merged 16 commits into from
Sep 11, 2024
Merged

Conversation

RODO94
Copy link
Contributor

@RODO94 RODO94 commented Sep 4, 2024

What does this PR do?

This PR follows work completed in: #3604

I am writing tests for the Error and Validation describe() as part of larger work to write tests for all of the test.todos().

My plan is to push up a PR for each describe() section so I can deal with feedback as I go and hopefully improve / fix any structure or logic issues.

For error checking at the Tab, I went with checking for the border-left styling as I couldn't see anything more specific to work from. From here I checked that this is removed when a new feature is added but is reapplied when you click "Continue" without adding new inputs.

The original test.todos there was one for Maximum number as an inverse to the minimum it() block I have written, but I didn't think there was a maximum for this component so I removed it.

Copy link

github-actions bot commented Sep 4, 2024

Removed vultr server and associated DNS entries

expect(map).toBeInTheDocument();
debug();

addMultipleFeatures([point1, point2]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new function for adding multiple features added here

import { props } from "../test/mocks/Trees";
import { addFeaturesToMap } from "../test/utils";

const addMultipleFeatures = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a function to call for when we want multiple features added to the map, thought it would be an easier spot in future if there was an explicit function for it.

It uses the existing function for adding one feature but wraps it in a forEach with a temp Array there to feed in all subsequent arrays.

export const props: PresentationalProps = {
title: "Mock title",
description: "Mock description",
schemaName: "Trees",
fn: "MockFn",
schema: Trees,
schema: mockTreeSchema,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mockTreeSchema was used here to remove the Date fields from the input. This was causing issue when we were trying to test for error messages and all of them being emptyDOMElements. We thought it was better to remove this since, as an input, it has test coverage outside of this.

@RODO94 RODO94 changed the title feat[mal]: Working through Map and Label index.test todos feat[mal]: Map and Label Error and Validation Tests Sep 10, 2024
@RODO94 RODO94 marked this pull request as ready for review September 10, 2024 14:38
@RODO94 RODO94 requested a review from a team September 10, 2024 14:38
@@ -16,3 +17,14 @@ export const addFeaturesToMap = async (
});
act(() => map.dispatchEvent(mockEvent));
};

export const addMultipleFeatures = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought an explicit function for adding multiple features would help with the issue of how we mock the map in the tests. Lower the chance of the issues I had earlier... utils felt like the right place and tried to keep it simple.

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Looks like a great start on more test coverage - a few comments to address/clean up before merging please!


const errorMessage = within(errorWrapper).getByText(/You must plot /);

expect(errorMessage).not.toBeEmptyDOMElement();
Copy link
Member

@jessicamcinchak jessicamcinchak Sep 10, 2024

Choose a reason for hiding this comment

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

nit: I think toBeInTheDocument() would be a stronger check for visibility here? Per a11y requirements, we have many error wrappers that are always in the DOM, and their visibility is toggled depending on component error state.

Copy link
Member

@jessicamcinchak jessicamcinchak Sep 11, 2024

Choose a reason for hiding this comment

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

@RODO94 re-reviewing now - if this suggested change worked as expected here, can you also please update all other similar instances of DOMElement checks as well? Otherwise I think these are quite flakey tests not actually testing visibility of error messages like we intend?

Copy link
Contributor Author

@RODO94 RODO94 Sep 11, 2024

Choose a reason for hiding this comment

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

@jessicamcinchak for this, would it be the case that for the Input fields in the <TabPanels> that the errors are always in the document but just don't have any text in them so toBeInTheDocument() would come back yes even if there's no error messages displaying?

Would something like this be more specific?

    errorMessages.forEach((message) => {
      expect(message).toHaveTextContent(/(Enter|Select)/);
    });
    
    || or ||
    
    errorMessages.forEach((message) => {
      expect(message.textContent).not.toEqual(undefined);
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example here of error element before "Continue" is clicked:

<p class="..." id="error-message-input-text-species" data-testid="error-message-input-text-species"></p>

Copy link
Member

Choose a reason for hiding this comment

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

Looking through other error handling tests in other components now and realising checking DOM element is used elsewhere too, let's leave this as-is right now. Sorry for confusion.

There's still many outstanding to-do's here that are more valuable to move onto than get hung up on this.

@RODO94 RODO94 merged commit 579c7ba into main Sep 11, 2024
12 checks passed
@RODO94 RODO94 deleted the rory/mal-test-todos branch September 11, 2024 12:58
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.

2 participants