Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

ADD: coverage for VueWrapper #98

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fanyer
Copy link

@fanyer fanyer commented Oct 21, 2019

Summary

Add a test case of VueWrapper . The scope It covers lists below: https://github.com/akxcv/vuera/blob/master/src/wrappers/Vue.js#L85-L93
https://github.com/akxcv/vuera/blob/master/src/wrappers/Vue.js#L38-L40

[BEFORE]
image

[AFTER]
image

Anything else relevant?
For current situation of vuera , this case will fail. I'm trying to fix it in the next PR.

When to meet this case?
When using a wrapper component, the component filed is passed in curly braces of JSX, so it can be dynamicly changed in setSate operation. However this case won't occur in HOC mode or babel plugin mode.

@fanyer fanyer mentioned this pull request Oct 21, 2019
@fanyer fanyer force-pushed the fye/feature/coverage branch from 5337d67 to 81c974d Compare October 21, 2019 20:20
@fanyer
Copy link
Author

fanyer commented Oct 22, 2019

I've tried to change component dynamicly in a real demo, not in jest. It works correctly. I'll take a look at it again.

@fanyer
Copy link
Author

fanyer commented Oct 22, 2019

Same case also occurs in https://github.com/akxcv/vuera/blob/master/tests/wrappers/VueWrapper-test.js#L122-L126 .

the original case is

it('synchronises props', () => {
    const reactAppInstance = makeReactInstanceWithVueComponent(VueComponent)
    reactAppInstance.setState({ message: 'New message!' })
    expect(document.body.innerHTML).toContain('New message!')
  })

However, I think toContain checks too loosely.
if the test is more strict, like below

 it('synchronises props', () => {
    const reactAppInstance = makeReactInstanceWithVueComponent(VueComponent)
    reactAppInstance.setState({ message: 'New message!' })
    expect(document.body.innerHTML).toBe(
      normalizeHTMLString(
      `<div id="root">
          <div>
            <input type="text" value="New message!">
            <div>
              <span>New message!</span> <button></button>
            </div>
          </div>
        </div>`
      )
    )
  })

It sync props failed actually (in jest test case, while it works correctly in real situation).

When setState({message}) , its real htmlString output should contain <span>New message!</span> , and the original case just determin whether New message! exists, which might appear in value="New message!"

image

I think some cases written before might lack of accuracy as well, esspeically The code left without coverage (seems only 2 warpers)

@fanyer
Copy link
Author

fanyer commented Oct 27, 2019

This PR will raise VueWrapper coverage from 19/23 to 23/23, and the total coverage percent from 90% to 93% @akxcv

@timsayshey
Copy link

@akxcv LGTM

* because the component filed is passed in curly braces of JSX,
* and it can be changed in `setSate` operation.
* However it won't occur in HOC mode or babel plugin mode.
* this test will fail. I'm trying to fixed it.
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't it fixed already?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants