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

Clean up test from #1915 #1933

Merged
merged 2 commits into from
Apr 26, 2019

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Apr 26, 2019

Cleaning up test from #1915:

  • Only put things that are useful for > 80% of tests in /__tests__/setup/web/index.html
    • This one does not hit the bar because turning the focus yellow is only meaningful for one of the many test cases
    • Try very best not to touch the DOM when doing setup
  • Put the test in its own file
    • basic.js is for really basic stuff, like, if Web Chat can be loaded or not (i.e. setup test)
    • basic.js acts as the first gate to check if everything is broken
    • More clean up over basic.js need to be done later
  • Added createStyleSet option to main()
    • This will allow other tests modify style set, in addition to style options
    • If tests prefer, they can pass styleSet instead of the new createStyleSet function
    • styleSet is designed to be serializable with flat hierarchy
    • We use createStyleSet here because we want the new style set to based on Web Chat's default, i.e. need to call window.WebChat.createStyleSet(styleOptions), then customize the result
    • We can't use spread/rest because the way the file is compiled, Object.assign is used instead
  • Test name should help debug future regressions
    • Our tests are based on stories, i.e. what user expect to work
    • Using story summary as test name is a good start for finding a good name

@coveralls
Copy link

Coverage Status

Coverage remained the same at 57.502% when pulling 832d9e5 on compulim:update-sendbox-focus-test into 8c8e2cc on Microsoft:master.

@corinagum corinagum merged commit 2c9a659 into microsoft:master Apr 26, 2019
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.

3 participants