test(text-helper): add test for emojifyText, abbreviateNumber by ibotdotout · Pull Request #568 · gitpoint/git-point

ibotdotout · 2017-10-23T15:59:45Z

#518

alejandronanez

Hey @ibotdotout thanks for your PR, can you please:

Remove nested describes
emojifyText should be tested in another way, we should mock the emoji package and then check if we called emoji.emojify with the right params. Right now, if emoji changes one of its contents this test will break.
Please add const to each constant/variable for example, expected = ... should be const expected = ....

andrewda

Looks good! Just a couple small changes.

Also @alejandronanez – I talked about this in Gitter, but I think nested describes are fine in this context.

andrewda · 2017-10-24T01:48:46Z

__tests__/tests/utilities/text-helper.js

+
+describe('Text Helper', () => {
+  describe('emojifyText', () => {
+    it('should get correctly display :caffee: with emoji', () => {


:caffee: => :coffee: 😉

andrewda · 2017-10-24T01:49:06Z

__tests__/tests/utilities/text-helper.js

+  describe('emojifyText', () => {
+    it('should get correctly display :caffee: with emoji', () => {
+      expected = 'I need more ☕️';
+      result = emojifyText('I need more :coffee:');


Declare these with const.

andrewda · 2017-10-24T01:49:33Z

__tests__/tests/utilities/text-helper.js

+    it('should get 1 when give 1', () => {
+      input = 1;
+      expected = 1;
+      result = abbreviateNumber(input);


Declare these with const.

andrewda · 2017-10-24T01:49:39Z

__tests__/tests/utilities/text-helper.js

+    it('should get 1k when give 1000', () => {
+      input = 1000;
+      expected = '1k';
+      result = abbreviateNumber(input);


Declare these with const.

andrewda · 2017-10-24T01:49:47Z

__tests__/tests/utilities/text-helper.js

+      input = 1100;
+      expected = '1.1k';
+      result = abbreviateNumber(input);
+      expect(result).toEqual(expected);


Declare these with const.

andrewda · 2017-10-24T01:50:42Z

__tests__/tests/utilities/text-helper.js

+      expected = '1.1k';
+      result = abbreviateNumber(input);
+      expect(result).toEqual(expected);
+    });


I would also add a test for something huge and random like 96234, which should equal 96.2k I think.

- mock emojifyText instead direct test output - insert const for variables - add huge and random 96234 -> 96.2k

coveralls · 2017-10-24T03:01:53Z

Changes Unknown when pulling c54a9cc on ibotdotout:utility-tests into ** on gitpoint:master**.

ibotdotout · 2017-10-24T03:11:17Z

thank you @alejandronanez @andrewda for your reviewed.

I improve

mock emojifyText instead direct test output, I not sure with sinon.spy that it correct ?
insert const to variables
add huge and random 96234 -> 96.2k

lex111 · 2017-10-24T12:16:23Z

@ibotdotout coluld you add a new line before expect?

coveralls · 2017-10-24T12:33:50Z

Changes Unknown when pulling 0a4ef41 on ibotdotout:utility-tests into ** on gitpoint:master**.

lex111 · 2017-10-24T12:38:08Z

__tests__/tests/utilities/text-helper.js

+  describe('emojifyText', () => {
+    it('should call correcly with text params', () => {
+      const emojify = sinon.spy(emoji, 'emojify');
+      const input = 'I need more :coffee';


Is there not at the end :?

alejandronanez

Can you please remove sinon and use jest instead? You can do spies and mocks with jest out of the box.

Thanks!!!

coveralls · 2017-10-24T12:48:54Z

Changes Unknown when pulling c7af126 on ibotdotout:utility-tests into ** on gitpoint:master**.

ibotdotout · 2017-10-24T12:57:22Z

@alejandronanez already change to use jest for spy with jest.spyOn.

coveralls · 2017-10-24T13:01:33Z

Changes Unknown when pulling 90467ea on ibotdotout:utility-tests into ** on gitpoint:master**.

andrewda

One more little wording change then 👍

andrewda · 2017-10-24T18:28:24Z

__tests__/tests/utilities/text-helper.js

+  });
+
+  describe('abbreviateNumber', () => {
+    it('should get 1 when give 1', () => {


Change this to: 'should return 1 when given 1'

andrewda · 2017-10-24T18:28:33Z

__tests__/tests/utilities/text-helper.js

+      expect(result).toEqual(expected);
+    });
+
+    it('should get 1k when give 1000', () => {


Apply same pattern as above

andrewda · 2017-10-24T18:28:39Z

__tests__/tests/utilities/text-helper.js

+      expect(result).toEqual(expected);
+    });
+
+    it('should get 1.1k when give 1100', () => {


andrewda · 2017-10-24T18:28:42Z

__tests__/tests/utilities/text-helper.js

+      expect(result).toEqual(expected);
+    });
+
+    it('should get 96.2k when give 96234', () => {


alejandronanez · 2017-10-25T02:01:37Z

__tests__/tests/utilities/text-helper.js

+describe('Text Helper', () => {
+  describe('emojifyText', () => {
+    it('should call correcly with text params', () => {
+      const emojify = jest.spyOn(emoji, 'emojify');


coveralls · 2017-10-25T02:40:51Z

Changes Unknown when pulling fd2b6a6 on ibotdotout:utility-tests into ** on gitpoint:master**.

done

test(text-helper): add test for emojifyText, abbreviateNumber

5f6376a

alejandronanez requested changes Oct 24, 2017

View reviewed changes

andrewda requested changes Oct 24, 2017

View reviewed changes

test(text-helper): improve it

c54a9cc

- mock emojifyText instead direct test output - insert const for variables - add huge and random 96234 -> 96.2k

andrewda approved these changes Oct 24, 2017

View reviewed changes

test(text-helper): add new line before expect

0a4ef41

lex111 reviewed Oct 24, 2017

View reviewed changes

test(text-helper): add new line after variables

c7af126

alejandronanez previously requested changes Oct 24, 2017

View reviewed changes

test(text-helper): remove sinon -> jest for mock

90467ea

chinesedfan added the tests label Oct 24, 2017

andrewda requested changes Oct 24, 2017

View reviewed changes

alejandronanez reviewed Oct 25, 2017

View reviewed changes

test(text-helper): clean test case title

fd2b6a6

andrewda approved these changes Oct 25, 2017

View reviewed changes

lex111 approved these changes Oct 25, 2017

View reviewed changes

andrewda merged commit 0cd67ba into gitpoint:master Oct 25, 2017

andrewda mentioned this pull request Oct 25, 2017

Add more tests! #518

Open

63 tasks

Conversation

ibotdotout commented Oct 23, 2017

Uh oh!

alejandronanez left a comment

Choose a reason for hiding this comment

Uh oh!

andrewda left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 24, 2017

Uh oh!

ibotdotout commented Oct 24, 2017

Uh oh!

lex111 commented Oct 24, 2017

Uh oh!

coveralls commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alejandronanez left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 24, 2017

Uh oh!

ibotdotout commented Oct 24, 2017

Uh oh!

coveralls commented Oct 24, 2017

Uh oh!

andrewda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andrewda left a comment •

edited

Loading