Skip to content

Refactor PO Search test suite to use wrapper option#7737

Merged
allthesignals merged 1 commit intomainfrom
wmg/test-cleanup
Jan 31, 2023
Merged

Refactor PO Search test suite to use wrapper option#7737
allthesignals merged 1 commit intomainfrom
wmg/test-cleanup

Conversation

@allthesignals
Copy link
Contributor

@allthesignals allthesignals requested review from a team and JackRyan1989 January 31, 2023 20:25
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@JackRyan1989 JackRyan1989 left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for taking care of this Matt. Just one small question but it shouldn't hold up approval/merge.

Tests pass in dev env.


describe('InPersonLocationStep', () => {
const wrapper: ComponentType = ({ children }) => (
<InPersonContext.Provider value={{ arcgisSearchEnabled: true }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the ordering of the InPersonContext.Provider and the SWRConfig reversed in the wrapper as opposed to the original code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh. Hmm. I don't know! I should've kept the original order, but I don't believe order hierarchy affects anything as far as context/providers go (unless there's some promise stuff going on). Unless you have some weird code that is sensitive to timing and re-renders and so on...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't think this is an issue here, but it seems like values could be overwritten:
"The Provider component accepts a value prop to be passed to consuming components that are descendants of this Provider. One Provider can be connected to many consumers. Providers can be nested to override values deeper within the tree." from React Docs.

@allthesignals allthesignals merged commit 530bdb1 into main Jan 31, 2023
@allthesignals allthesignals deleted the wmg/test-cleanup branch January 31, 2023 21:15
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