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

test: refactor button component test with user event and screen #36926

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

Conversation

clean99
Copy link
Contributor

@clean99 clean99 commented Aug 6, 2022

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

Problems:

  1. Overusing snapshot tests: There are thousands of snapshot tests everywhere, the disadvantages are:
  • Low readability: Almost nobody will go the snapshot files to look at the snapshot with they don't know what it means.
  • False negative: snapshot tests for dom is a kind of testing implementation, because it will rule that those class, dom structure need to be exactly the same. But the user don't care about what class is been add in the element. So some refactor of style or replace a div element use section will make the tests fail while the result is correct.
  • Bad documentation: when we use assertion like expect(a).toBeIndocument(), we are telling the reader that the document have a element, but expect(doc).toMatchSnapshot() really just unclearly and reader likely can't get the point(about what is important and what is not) of the test.
  1. QuerySelector everywhere: querySelector really can't give you the confidence as much as screen.getBy.... Because if you have to use querySelector to get an element, that usually means that it is unaccessible for user. What we need to do is to write test that simulate user as far as possible.
  2. fireEvent: I recommend userEvent instead of using fireEvent since the userEvent simulate user more than fireEvent. Like the fireEvent.change event, it only target change event while the userEvent.type will target focus, change, keyboard down and so on.
  3. Created Test user: in the should change loading state instantly by default case, it create a component that wrap the button and then use state and setState to change the button loading status. Now we are testing not only the button, but also a test component and react's api, so if one of them failed, the test will failed for sure. Test should be isolated. I think we should always test the interface, instead of a testing component.

Solution
I change most of the snapshot tests using the screen query way, and replace querySelector too. Also switch fireEvent to userEvent. And refactor the should change loading state instantly by default test to only test it change loading status when the props change.

📝 Changelog

Language Changelog
🇺🇸 English Replace querySelector, snapshot tests with screen query. Replace fireEvent with userEvent. Refactor incorrectly test case.
🇨🇳 Chinese 把button测试中的query Selector,snapshot换成screen query。替换fireEvent为userEvent。重构不合适的测试用例。

☑️ Self-Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2022

@codecov
Copy link

codecov bot commented Aug 6, 2022

Codecov Report

Merging #36926 (cebf13a) into master (20a2bb3) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master    #36926   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          437       437           
  Lines         8035      8035           
  Branches      2420      2420           
=========================================
  Hits          8035      8035           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@afc163
Copy link
Member

afc163 commented Aug 17, 2022

@zombieJ @MadCcc 看看

@clean99
Copy link
Contributor Author

clean99 commented Aug 22, 2022

@afc163 @zombieJ @MadCcc Hi大佬们,请问有时间可以帮我看看这个pr吗?我还想继续帮忙写测试但是怕写得不好。。

@zombieJ
Copy link
Member

zombieJ commented Aug 24, 2022

@clean99 ,最近社区同学在迁移 testing lib 容易其冲突,加一下钉钉群一起共建不~~

#34087 (comment)

@clean99
Copy link
Contributor Author

clean99 commented Aug 25, 2022

好的👌,我加一下~

@afc163
Copy link
Member

afc163 commented Oct 24, 2023

冲突了

1 similar comment
@afc163
Copy link
Member

afc163 commented Jan 19, 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.

None yet

3 participants