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

fix(web): add forgotten ReactRouter#useHref mock #1537

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Aug 14, 2024

#1536 faced some problems in CI not reproducible locally resulting in a set of changes in the test suite to make CI happy.

But these CI complaints were right since a mock for ReactRouter#useHref hook was forgotten and not sent as part of the changes, reason why initial tests were not working.

Now we have to decide: to go ahead with this PR and continue mocking ReactRouter for these simple tests or to keep previous tests which were not mocking mentioned useHref hook. In any case, changes file does not need to have a new entry for this.

@ancorgs
Copy link
Contributor

ancorgs commented Aug 14, 2024

Well, mocking useHref looks more consistent with all the other related tests. Isn't it?

@dgdavid
Copy link
Contributor Author

dgdavid commented Aug 14, 2024

Well, mocking useHref looks more consistent with all the other related tests. Isn't it?

Yes, it is.

Just to be clear, I'm all for the mocking here because I expect that behavior of useHref and others ReactRouter hooks are already tested by the library. So, I don't care about their output but that the component in which they are used use such a value. Thus, mocking is perfectly fine to me here. But I had to ask just in case you guys has a different view.

Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM

@dgdavid dgdavid merged commit 447a124 into master Aug 14, 2024
2 checks passed
@dgdavid dgdavid deleted the forgotten-mock branch August 14, 2024 12:35
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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