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

[WIP] Rewrite toast unit tests #28562

Closed
wants to merge 7 commits into from
Closed

[WIP] Rewrite toast unit tests #28562

wants to merge 7 commits into from

Conversation

bershanskiy
Copy link
Contributor

@Johann-S Here are Toast unit tests.

  • I'm still working on two tests (marked with TODO), I'll finish them tomorrow
  • This time I didn't try to add any new unit tests

@bershanskiy bershanskiy requested a review from a team as a code owner March 27, 2019 07:08
@bershanskiy bershanskiy changed the title Rewrite toast unit tests [WIP] Rewrite toast unit tests Mar 27, 2019
@Johann-S
Copy link
Member

Just few notes:

  • Do not exclude js/dist in the .gitgnore
  • You have to do one describe per methods

When you're done with that just /CC me 😉

I'll update my branch soon, so to keep up to date you can do:
git pull upstream jo-jasmine --rebase

upstream is the remote which target Bootstrap

.gitignore Outdated Show resolved Hide resolved
@Johann-S Johann-S force-pushed the jo-jasmine branch 3 times, most recently from f2e0f87 to a81f11a Compare March 27, 2019 20:30
@bershanskiy
Copy link
Contributor Author

@Johann-S Thanks for your comments. I tried to fix all the issues and squashed all commits into one, I hope it's easier to review this way.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

It lacks of DefaultType and Default describe

js/src/toast/toast.spec.js Outdated Show resolved Hide resolved
js/src/toast/toast.spec.js Outdated Show resolved Hide resolved
js/src/toast/toast.spec.js Outdated Show resolved Hide resolved
js/src/toast/toast.spec.js Outdated Show resolved Hide resolved
js/src/toast/toast.spec.js Outdated Show resolved Hide resolved
js/src/toast/toast.spec.js Outdated Show resolved Hide resolved
@Johann-S Johann-S self-requested a review April 3, 2019 11:46
@Johann-S
Copy link
Member

Johann-S commented Apr 3, 2019

Hi @bershanskiy,

I fixed some lint error on your commit.

You have one method left to test:

image

And I'll be happy to merge your PR 👍
Thanks again 👍

@Johann-S Johann-S force-pushed the jo-jasmine branch 7 times, most recently from 642cb93 to ec988c4 Compare April 10, 2019 08:48
@Johann-S
Copy link
Member

Hi @bershanskiy I cherry picked your commit in my branch, thanks for your work 👍

@Johann-S Johann-S closed this Apr 10, 2019
@Johann-S Johann-S mentioned this pull request Apr 10, 2019
21 tasks
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