Align tests with latest approaches and style guides. While doing so also re-evaluate each test and add/remove ones as necessary.

[erangeles]

sl-translate testing suite evaluation

services/sl-translate.js	test evaluation
properties
dictionary	1 test (default value test)
methods
setDictionary()	2 tests(type acceptance test of object, setting correctly test)
getKeyValue()	2 tests(test correct return value) -> merged into 1 test (both testing correct and incorrect return value)
translateKey()	6 tests(type acceptance test of object, test correct return value, pluralKey/pluralCount param test, pluralCount type test, pluralization count number test, token replacement test ) -> refactored type acceptance test to follow style/convention guide, refactored testing correct return value, refactored pluralCount type test to test against all types

mixins/sl-get-translation.js	test evaluation
Services
translateService	2 test(service being injected properly, mixin successfully)
methods
get()	2 tests(translate prefix param test)
translate()	1 test(returns getKeyValue call)

components/sl-translate.js	test evaluation
properties
internalTranslatedString	0 tests -> added default value test
tagName	1 test(render correct tag)
methods
setTranslatedStringUponRender()	1 test( willRender lifecycle trigger runs setTranslatedString) -> refactored test to trigger 3 willRender lifecycle hooks to test multiple runs of setTranslatedString
setTranslatedString()	2 tests(sets correct translatedString from the translateString() method, correct return value test)
translateString()	2 tests( calls translateKey with correct values, regex pattern param test for param keys)
translatedString()	1 tests( test for correct dependentKeys)

[notmessenger]

Is https://github.com/softlayer/sl-ember-translate/issues/124 addressed by this PR as well?

[notmessenger]

• Is the "Used Alongside Other Properties" component integration test case still needed now that observers are not being manually added/removed to emulate the desired binding behavior?
• Via https://github.com/softlayer/ember-style-guide/blob/master/ember-testing.md#component-integration-tests, components

  MUST reference a component under test via this.$( '>:first-child' )

• assert.expect() calls should be removed everywhere not needed in /tests/unit/mixins/sl-get-translation-test.js

---

Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

---

^{tests/unit/components/sl-translate-test.js, line 30 [r1] (raw file):} This test should be approached differently.

---

^{tests/unit/components/sl-translate-test.js, line 66 [r1] (raw file):} What is the value in triggering this thrice?

---

^{tests/unit/components/sl-translate-test.js, line 104 [r1] (raw file):} This test should only be testing that translateString() returns the expected value. There should be a separate test for translatedString()'s functionality.

---

^{tests/unit/services/sl-translate-test.js, line 23 [r1] (raw file):} Can the requires() helper be employed here, with the addition of a Symbol assertion alongside it?

---

^{tests/unit/services/sl-translate-test.js, line 117 [r1] (raw file):}

• This test case should assert that when a key that does not exist in the dictionary is requested the requested key name is returned. If it does exist in the dictionary than the value of the key is returned.
• Update the name of this test case to reflect this behavior.

---

^{tests/unit/services/sl-translate-test.js, line 141 [r1] (raw file):} What is the reasoning for removing use of the requires() helper? All that should have had to be done was add an additional test alongside its use to assert for Symbol.

---

^{tests/unit/services/sl-translate-test.js, line 191 [r1] (raw file):} The idea behind this test case was to prove/ensure that you had to pass the value of 2 instead of two, for example, that there was nothing in the code converting spelling to numbers. When this property is used in translateKey() it is cast as a Number so do not believe the additional type assertions you have added are necessary. If anything, perhaps this entire test case should be removed.

---

^{tests/unit/services/sl-translate-test.js, line 220 [r1] (raw file):} Haven't dug into it thoroughly but thinking this doesn't need to be asserted. As long as a test case somewhere in the test suite is asserting that getKeyValue() gets called by translateKey() then the getKeyValue() tests are taking care of this.

---

Comments from the review on Reviewable.io

[erangeles]

124 will be addressed in a different change set.

---

Review status: 0 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks pending.

---

^{tests/unit/components/sl-translate-test.js, line 30 [r1] (raw file):} Done.

---

^{tests/unit/components/sl-translate-test.js, line 66 [r1] (raw file):} deleted.

---

^{tests/unit/components/sl-translate-test.js, line 104 [r1] (raw file):} Done.

---

^{tests/unit/services/sl-translate-test.js, line 23 [r1] (raw file):} done, addition of Symbol not added due to https://github.com/softlayer/sl-ember-components/issues/469 . We currently don't test for symbol in all our sl-ember repos due to non-support in phantomJS version.

---

^{tests/unit/services/sl-translate-test.js, line 117 [r1] (raw file):} Done.

---

^{tests/unit/services/sl-translate-test.js, line 141 [r1] (raw file):} done, addition of Symbol not added due to https://github.com/softlayer/sl-ember-components/issues/469 . We currently don't test for symbol in sl-ember-components due to non-support.

---

^{tests/unit/services/sl-translate-test.js, line 191 [r1] (raw file):} Done.

---

^{tests/unit/services/sl-translate-test.js, line 220 [r1] (raw file):} Done.

---

Comments from the review on Reviewable.io

[erangeles]

@notmessenger I'm currently stuck on the failing test that you see on the build, I wanted to see if you can give it a look please. So basically I employ the requires helper to assert that setDictionary() only accepts an object as a param. As you see the assert statements are the same in both translateKey() and setDictionary() . http://pastebin.com/28usEsym this pastebin is from my browser console, I call the method with all the arguments and it throws an exception in all cases but the object type passed in. So I'm not sure why the requires method isnt working.

---

Review status: 0 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[notmessenger]

The approached used in tests/unit/components/sl-translate-test.js for the testing of default property values needs to align with the style guide and examples in use in our other repos.

Go ahead and remove the use of the requires helper - it will be easier for now.

---

Reviewed 8 of 8 files at r2. Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

^{addon/services/sl-translate.js, line 49 [r2] (raw file):} If this error is displayed in our console while developing are you able to discern the location from which it was thrown? This is the reason this message is/was specific. If it can be easily discerned then I am okay with the proposed modification.

---

^{tests/unit/components/sl-translate-test.js, line 120 [r2] (raw file):} Why do we need a spy? Can't we just call component.translateString() directly?

---

Comments from the review on Reviewable.io

[erangeles]

Review status: 5 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks pending.

---

^{addon/services/sl-translate.js, line 49 [r2] (raw file):} Done.

---

^{tests/unit/components/sl-translate-test.js, line 120 [r2] (raw file):} Done.

---

Comments from the review on Reviewable.io

[notmessenger]

If reverting use of the requires helpers, revert the changes to package.json, tests/.jshintrc, and tests/helpers/start-app.js. The change to package.json can actually the changes required by https://github.com/softlayer/sl-ember-translate/issues/181 and its associated issue and can resolve a 3 issues with this PR.

---

Reviewed 3 of 3 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{tests/unit/components/sl-translate-test.js, line 30 [r1] (raw file):} There should be a single "Default property values" test case that asserts the service name, tagName, and internalTranslatedString values.

---

^{tests/unit/services/sl-translate-test.js, line 118 [r3] (raw file):} Why were these two previous test cases combined? If going to leave as such, modify the description, at least the use of "if existent"

---

Comments from the review on Reviewable.io

[erangeles]

@notmessenger reverted changes and will address #181 in it's own change set.

---

Review status: 3 of 5 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

^{tests/unit/components/sl-translate-test.js, line 30 [r1] (raw file):} Done.

---

^{tests/unit/services/sl-translate-test.js, line 118 [r3] (raw file):} Done.

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 2 of 5 files at r4. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[notmessenger]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{addon/services/sl-translate.js, line 49 [r4] (raw file):} to what?

---

^{addon/services/sl-translate.js, line 107 [r4] (raw file):} to what?

---

Comments from the review on Reviewable.io

[erangeles]

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{addon/services/sl-translate.js, line 49 [r4] (raw file):} Done.

---

^{addon/services/sl-translate.js, line 107 [r4] (raw file):} Done.

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 1 of 1 files at r5. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io