Closes softlayer/sl-ember-test-helpers#156

[theoshu]

[notmessenger]

• The testing story needs to be improved. Aziz and I spoke and he knows what is desired.
• Have you confirmed the operation of these helpers in an async environment (application acceptance tests)?
  ** If confirmed add entry to async section of docs
  ** If not desired, how enforcing not usable in this fashion?
• The importing of sinon in test-support/helpers/sl/synchronous/global-libraries.js assumes sinon is installed. Should this addon force that upon a user during its installation? Is it only enforced via documentation that it is required? Is there a way to determine than an import will or did fail so that if we are not installing sinon into an app (if that's the path we take) and a user ignores the docs and uses the helper any way we can give a message as to why? Perhaps too much work but worth thinking about.

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

CHANGELOG.md, line 5 [r1] (raw file):

• Neither of the numbers listed correspond to the issue number this PR is against.
• Wrap "globalLibraries" text with back ticks

---

README.md, line 104 [r1] (raw file):
Speak to how to use these helpers if how the value of called is assigned and need more than that.

---

Comments from the review on Reviewable.io

[notmessenger]

Have you confirmed the operation of these helpers in an async environment (application acceptance tests)?

• If confirmed add entry to async section of docs
• If not desired, how enforcing not usable in this fashion?

---

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

---

CHANGELOG.md, line 5 [r4] (raw file):
The number and link should both reflect #158

---

index.js, line 10 [r4] (raw file):

• This is needed for the testing environment, not development
• Have you confirmed that this file does not end up being built in the a consuming application's vendor.js file?

---

test-support/helpers/sl/synchronous/global-libraries.js, line 16 [r4] (raw file):
https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#block-statements

---

test-support/helpers/sl/synchronous/global-libraries.js, line 22 [r4] (raw file):
https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#block-statements

---

tests/unit/helpers/sl/synchronous/global-libaries-test.js, line 92 [r4] (raw file):
What about a test that called() returns false when nothing is referenced?

---

Comments from the review on Reviewable.io

[notmessenger]

Build is failing

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[azizpunjani]

tests/unit/helpers/sl/synchronous/global-libaries-test.js, line 92 [r4] (raw file):
Jeremy, doesn't this test what you are mentioning?

---

Comments from the review on Reviewable.io

[azizpunjani]

tests/unit/helpers/sl/synchronous/global-libaries-test.js, line 92 [r4] (raw file):
I guess it's not isolated enough because it triggers events?

---

Comments from the review on Reviewable.io

[notmessenger]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

tests/unit/helpers/sl/synchronous/global-libaries-test.js, line 92 [r4] (raw file):
Maybe change the name of the test to align with the references to called() like in the other ones asserting the opposite state?

---

Comments from the review on Reviewable.io

[notmessenger]

index.js, line 10 [r4] (raw file):
http://ember-cli.com/user-guide/#test-assets gives some approaches for this scenario

---

Comments from the review on Reviewable.io

[azizpunjani]

@notmessenger this is ready for review when you have a chance.

[notmessenger]

• Have you confirmed the operation of these helpers in an async environment (application acceptance tests)?
• If above is confirmed add entry to async section of docs
• If above is not desired, how are we enforcing it is not usable in this fashion?
• What about an uninstall story, that uninstalls the ember-sinon package? What if that package is being used by something else?
• Nothing in the documentation alerts the user that the ember-simon package will be installed during this add-on's installation. Should there be?
• There are still outstanding comments from previous revisions that have not been addressed.

---

Reviewed 2 of 2 files at r7.
Review status: 6 of 8 files reviewed at latest revision, 9 unresolved discussions.

---

CHANGELOG.md, line 5 [r10] (raw file):
The number and link should both reflect #156

---

index.js, line 10 [r4] (raw file):
I now realize that this link was actually guidance for the * ember-cli-build.js* file, which this is not.

---

README.md, line 90 [r10] (raw file):
Change "globally scoped" to "globally-scoped"

---

README.md, line 107 [r10] (raw file):
Change "willInsertElement, didInsertElement, willClearRender and willDestroyElement" to "willInsertElement, didInsertElement, willClearRender, and willDestroyElement"

---

README.md, line 108 [r10] (raw file):
"that that"?

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 1 of 2 files at r9.
Review status: 7 of 8 files reviewed at latest revision, 10 unresolved discussions.

---

blueprints/sl-ember-test-helpers/index.js, line 26 [r10] (raw file):
Did you try installing this into an app? This syntax will not be valid, as it is not transpired.

---

Comments from the review on Reviewable.io

[notmessenger]

Review status: 7 of 8 files reviewed at latest revision, 11 unresolved discussions.

---

blueprints/sl-ember-test-helpers/index.js, line 33 [r10] (raw file):
Should we use https://github.com/elwayman02/ember-sinon-qunit instead?

---

Comments from the review on Reviewable.io

[azizpunjani]

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

---

blueprints/sl-ember-test-helpers/index.js, line 26 [r10] (raw file):
This will be valid if they are running node version >= 4.0. Arrow functions were introduced in v4.0.0. Should we recommend a node version >=4.0.0? Or do we want to revert the code?

---

blueprints/sl-ember-test-helpers/index.js, line 33 [r10] (raw file):
I think for our use case we can do without it. Seems like another layer wrapping ember-sinon.

---

Comments from the review on Reviewable.io

[azizpunjani]

Reviewed 2 of 6 files at r1, 1 of 2 files at r7, 1 of 2 files at r9, 4 of 4 files at r11.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[azizpunjani]

@notmessenger this is ready for review.

[notmessenger]

Merge conflicts

---

Reviewed 4 of 4 files at r11, 1 of 1 files at r13.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

blueprints/sl-ember-test-helpers/index.js, line 26 [r10] (raw file):
I lean toward reversion as if we can easily accommodate all versions with a slight syntax change on our side we don't cause any burden to the user.

---

CHANGELOG.md, line 5 [r13] (raw file):
The number and link should both reflect #156

---

Comments from the review on Reviewable.io

[azizpunjani]

@notmessenger this is ready for review, made changes you suggested.

[notmessenger]

Reviewed 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[notmessenger]

There are a few things about this PR that still need to be changed but I am going to merge it and change them myself so we can get this put to bed.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io