Skip to content

(WIP) Exploration/prototyping of test utils package#4836

Closed
LFDanLu wants to merge 7 commits intomainfrom
test_utils
Closed

(WIP) Exploration/prototyping of test utils package#4836
LFDanLu wants to merge 7 commits intomainfrom
test_utils

Conversation

@LFDanLu
Copy link
Copy Markdown
Member

@LFDanLu LFDanLu commented Jul 27, 2023

Closes

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

RSP

Copy link
Copy Markdown
Member

@jluyau jluyau left a comment

Choose a reason for hiding this comment

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

i think these utils would definitely make writing tests for rsp components much cleaner. another utility class that would help would be interacting with the current dialog (methods to get elements within the content body as well as pressing the dialog buttons)

);
};

beforeAll(function () {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is probably on your radar, but having these mocks and such callable with 1 line of code would be nice too!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, I've been mulling over whether or not these mocks are too broad. Ideally we would limit what it actually gets applied on, similar to

jest.spyOn(window, 'getComputedStyle').mockImplementation(element => {
if (element.attributes.getNamedItem('data-testid')?.value === 'container') {
const sty = realGetComputedStyle(element);
sty.position = 'relative';
return sty;
} else {
return realGetComputedStyle(element);
}
})
, but I'm worried that users may need to have finer control over the mocks.

Perhaps we can provide a basic util that does the simple mock and offer a way to override/customize

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Another interesting idea that @devongovett brought up was making our virtualized components render as non-virtualized components when rendered in a test environment specifically. That means we won't have to mock any of this stuff, thus avoid possible mock collisions and assumptions. RSP would still write tests with the virtualized components still being virtualized just to make sure things work as expected

this.rows = within(this.rowgroups[1]).getAllByRole('row');
}

toggleRowSelection(opts: {index?: number, text?: string, needsLongPress?: boolean}) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one helper function that i think would help (since it's come up a bunch) is a method to select a given cell, though i guess it'd require the column and row indexes or text

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sounds good, we have a util for that that we use internally as well actually haha. I think we can start off with searching for cell text first since that is what a user would look for typically and see about supporting a (x row, y column) alternative maybe.

@LFDanLu
Copy link
Copy Markdown
Member Author

LFDanLu commented Aug 24, 2023

Closing this for now, will incorporate the feedback when work resumes on this portion of the testing util work

@LFDanLu LFDanLu closed this Aug 24, 2023
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