This repository was archived by the owner on May 29, 2019. It is now read-only.
test(alert): add more coverage and other minor changes #1489
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
3 proposed changes:
There are tests for classes and close buttons, but the content itself is not tested. We need to be sure that the content is there. For consistency, I added a method and made a for loop like the other on the test (which I find a little bit redundant). I find by span (interpolations are converted to span). I would prefer to add a class to the transclude div, but that is not my decision.
The test that tests the classes. It test the first two and then some defaults.
Im my opinion, it should test that the third one (which doesn't have a type associated) should contain an
alert-warningclass.On the other hand, I deleted the other expectations. All of them contains
alertso I don't see the need to test that and if you want to be sure that your alerts only have one type and no others, that could be tested in anotherit.Last, I removed a redundant "it".
Feel free to modify this or reject it at all :P