test: Added tests for auth reducer and refactored some other tests by jglover · Pull Request #579 · gitpoint/git-point

jglover · 2017-10-25T14:52:50Z

Adds test for auth reducer and refactoring.
Also adds default fallback for 'en' inside localization-helper incase location cannot be determined/promise to get locale in app.js has not fulfilled yet and somehow user has navigated, requiring a value (this was breaking unit tests).

coveralls · 2017-10-25T14:59:10Z

Coverage increased (+2.7%) to 33.584% when pulling 09d659f on jglover:tests/auth-reducer-tests into a43d3bc on gitpoint:master.

andrewda

End tests in .js, not .spec.js. Besides that looks good.

andrewda · 2017-10-25T18:20:15Z

Also - don't end data files in .data.js. If we're going to change the extensions for the files, we should do it in a new PR. For now keep things consistent.

andrewda · 2017-10-25T18:56:58Z

There's a lot of data in data/api/users.data.js file. Maybe we could shorten it down to 1 event per type so it's easier to follow if something breaks?

jglover · 2017-10-25T22:24:12Z

I'll rename those files, yeah it's quite a bit of data; I just took the GitHub example responses verbatim from the API docs. Since the file isn't used in a build is it a good idea to just keep data as a copy of the example responses incase it ever needs to be changed?

jglover · 2017-10-26T07:28:25Z

@andrewda done. What's the reasoning behind not using *.spec.js though? I'm curious about that one, it's the accepted convention in just about every framework or project I've worked with and helps differentiate the file when using fuzzy search, triggers test syntax highlighting in most editors etc

coveralls · 2017-10-26T07:39:13Z

Coverage increased (+2.3%) to 36.801% when pulling f042ae5 on jglover:tests/auth-reducer-tests into 6567aa3 on gitpoint:master.

coveralls · 2017-10-26T07:46:16Z

Coverage increased (+2.3%) to 36.801% when pulling f042ae5 on jglover:tests/auth-reducer-tests into 6567aa3 on gitpoint:master.

andrewda · 2017-10-26T13:52:11Z

@jglover Several reasons: 1) when I began adding tests, I didn't use it, and we've stayed consistent with that, 2) everything's already in a single __tests__/ directory, so it's not really necessary to define something as a specification. That being said, I'd love to completely redo our test naming scheme in another PR (an issue for discussion would be great, too).

alejandronanez

Besides @andrewda changes. LGTM

alejandronanez · 2017-10-27T01:29:37Z

src/utils/localization-helper.js

 export const getLocale = () => {
-  const locale = I18n.locale.toLowerCase();
+  // If for some reason a locale cannot be determined, fall back to 'en'.
+  const locale = (I18n.locale && I18n.locale.toLowerCase()) || 'en';


@jglover Please read en from src/config/common, as well as the comment.

@chinesedfan good tip, updated.

jglover · 2017-10-29T10:48:05Z

@andrewda Anything left outstanding? Keen to get this merged before it gets conflicts.

chinesedfan · 2017-10-29T11:12:23Z

src/utils/localization-helper.js

 export const getLocale = () => {
-  const locale = I18n.locale.toLowerCase();
+  // If for some reason a locale cannot be determined, fall back to 'en'.
+  const locale = (I18n.locale && I18n.locale.toLowerCase()) || 'en';


@jglover Please read en from src/config/common, as well as the comment.

coveralls · 2017-10-29T11:31:18Z

Coverage increased (+4.9%) to 39.359% when pulling bc82b7f on jglover:tests/auth-reducer-tests into 6567aa3 on gitpoint:master.

test: Added tests for auth reducer and refactored some other tests

09d659f

chinesedfan added the tests label Oct 25, 2017

andrewda requested changes Oct 25, 2017

View reviewed changes

andrewda mentioned this pull request Oct 25, 2017

Add more tests! #518

Open

63 tasks

test: renamed tests and data files

d330760

jglover added 3 commits October 26, 2017 07:30

Merge branch 'master' into tests/auth-reducer-tests

fd473dc

Merge branch 'master' into tests/auth-reducer-tests

088fde6

test: reverted test data back to default exports

f042ae5

alejandronanez approved these changes Oct 27, 2017

View reviewed changes

chinesedfan requested changes Oct 29, 2017

View reviewed changes

test: updated fallback in localization-helper to use value from common

bc82b7f

chinesedfan approved these changes Oct 29, 2017

View reviewed changes

andrewda approved these changes Oct 29, 2017

View reviewed changes

andrewda merged commit 2fda53e into gitpoint:master Oct 29, 2017

Conversation

jglover commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 25, 2017

Uh oh!

andrewda left a comment

Choose a reason for hiding this comment

Uh oh!

andrewda commented Oct 25, 2017

Uh oh!

andrewda commented Oct 25, 2017

Uh oh!

jglover commented Oct 25, 2017

Uh oh!

jglover commented Oct 26, 2017

Uh oh!

coveralls commented Oct 26, 2017

Uh oh!

coveralls commented Oct 26, 2017

Uh oh!

andrewda commented Oct 26, 2017

Uh oh!

alejandronanez left a comment

Choose a reason for hiding this comment

Uh oh!

alejandronanez Oct 27, 2017

Choose a reason for hiding this comment

Uh oh!

chinesedfan Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

jglover Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

jglover commented Oct 29, 2017

Uh oh!

chinesedfan Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jglover commented Oct 25, 2017 •

edited

Loading