Skip to content

Conversation

cuixiaorui
Copy link
Contributor

I extracted some duplicate code while reading the test file
But I don't know what you think about this code style

@lmiller1990
Copy link
Member

lmiller1990 commented Jul 13, 2020

Hi! Thanks for taking the time for this PR.

Regarding ShapeFlags:

The idea was for this library to have 0 dependencies. This makes the build step much more simple, as well as usage (for example, if you use vue-test-utils in the browser and have the @vue/shared dependency, the user needs to make sure that dependency is also available globally, which is not really ideal). Since ShapeFlags doesn't change much (if ever), I am not sure if there is much benefit in adding @vue/shared as a direct dependency.

Regarding the refactor in the tests

Although there is less code, I am not really a fan of this refactor. The component with the relevant class (class-a) is defined around line 8, however the relevant test is on line 47. In my opinion, each test should be "self contained", so you can look at the test and see all the relevant information, as opposed to scanning the file to find out what the component looks like.

Finally, I've found that enforcing immutability whereever possible leads to more understandable code. If you are constantly mutating and reassigning wrapper, it can lead to weird bugs where the value is reassigned accidentally.

For these reasons I don't see value in merging this PR. If you would like to pick up some work, though, I have some recommendations!

Some easy issues to pick up:

  • we have a mix of file names. we have camel-case.ts and camelCase.ts. We should pick one. You can rename them and preserve the git history with git mv. I'd reommend camelCase.ts, that is what Vue core is doing. This would be a very easy improvement to make.
  • We need some tests for functional components, there are not many (if any?) tests for those. See Add a few tests for Functional Components #103

Some other issues that need attention that will take a bit more effort:

  • Fix this PR: feat: setData #52. This is the setData feature - we don't want to use lodash since it makes the bundle huge, ideally we would write out own merge function and get all those tests to pass.
  • For something more ambitious, we are exploring ways to improve testing Vue Router. Issue: Vue Router test utils #152. Writing tests for the router is a bit painful now. Here is a working example of a Router test - I need to do lots of await.

If you want more help on these issues, let me know and I can provide some guidance.

@cuixiaorui
Copy link
Contributor Author

Thank you very much

I love testing, I'm a TDD practitioner, I want to help improve VTU,

I'll go and look at some of the PR that I mentioned

My English may not be very good, I will ask you if there is any question

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