Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

WIP: Add router and store access via utils returned from render function #53

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

Closed
brennj wants to merge 3 commits into testing-library:master from brennj:access-router-and-store

Conversation

@brennj
Copy link

@brennj brennj commented Jul 7, 2019

For #52

I do need some help with the router itself, it seems to stay onto the old instance. The new test added to tests/__tests__/vue-router.js will fail unless it is first executed, the router instance is still on the about route and I'm not sure of the cause of this. Any ideas from anyone would be appreciated!

maxpou reacted with thumbs up emoji
Copy link
Member

afontcu commented Jul 8, 2019

Hi John, and thank you for this PR!

I must say I'm a bit torn on this one. On the one hand, it makes sense to expose both router/store access to make some assertions given specific contexts. On the other, however, I feel this creates a distance between Vue Testing Library and the guiding principles that guides it.

I guess what I'm trying to say is that, given a route change, a test should assert that the page content has successfully changed, rather than checking it programmatically. Exposing both the router and (especially) the store instances are creating a simpler path to test implementation details, while creating yet another way to "test the same thing" (thus making the library a bit more complex).

What's your take on this? Can we think of some use cases that could not be covered by just checking the resulting action of navigating or interacting with the store as a user would?

Again, thank you for your work!

Copy link
Author

brennj commented Jul 8, 2019

I can understand your sentiment @afontcu. To me, it highlights a higher level grievance of the implementation details of the library is that the store and router initialization should not be included in the render method and left in user land so that the user can achieve the use cases I want.

I.e. wanting to understand the current URL. To me there is use cases of needing to use the router methods of using the history utilities to simulate the user using the URL bar as opposed to using application navigation. But that's not possible right now since they are baked in. I agree with you more on the store. But it goes hand in hand since they are in the details of render.

Copy link
Member

afontcu commented Jul 10, 2019

I was just checking react testing library examples of router tests (here and here), to see how they handle this in react land. From what I've seen, they actually interact with the app and then assert that the appropiate content is visible.

However, in the example linked above, they expose a history object (with a sensible warning about avoiding impl details) and use it (in reach router tests).

Do you think you could provide a valid example of a test use case where asserting content is not enough, and where you'd rather use some internal router features? Maybe add such tests in the PR.

Thanks! 🙌

(btw, I'd love to know your take on this @dfcook)

Copy link
Author

brennj commented Jul 21, 2019

I will get around to this, I've been trying to write some more tests for stuff in work and seeing if I'm feeling I'm missing it. The history object, as you say - is what I find myself wanting most of the time in the scenarios I want to 'look' at the url bar. I'm pretty conflicted TBH! 😅

afontcu reacted with thumbs up emoji

Copy link
Collaborator

dfcook commented Jul 30, 2019

I'd like to keep consistency with the other testing-library implementation where possible, let us know where your tests lead you.

afontcu reacted with thumbs up emoji

@afontcu afontcu added the wontfix This will not be worked on label Aug 18, 2019
Copy link
Author

brennj commented Sep 15, 2019

Closing for now, I haven't come across needing this properly yet.

Copy link
Member

afontcu commented Sep 15, 2019

Great, thanks! Feel free to open up a new open (or reopen this one) if you come across a valid use case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

wontfix This will not be worked on

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /