Testing with mocha #20

ghost · 2017-03-16T22:11:08Z

Story / Task

#4

What this PR does?

Adds the first set of testing using mocha

Before

We did not have tests

After

We have our first tests now 🎉

How to test / reproduce

Create a .env file on the root folder of this project with the following content

MONGODB_SERVICE_HOST=
MONGODB_TEST_DATABASE=
MONGODB_USER=
MONGODB_PASSWORD=

Fill the corresponding values
Execute npm run test
All tests should pass

Related PRs

N/A

TODOs

Review

Migrations

No

Thank you!

ghost

I'm leaving down my comments so we can discuss my doubts :)

ghost · 2017-03-16T22:16:07Z

package.json

@@ -10,7 +10,7 @@
    "build": "npm run lint && npm run compile",
    "prepublish": "npm run build",
    "lint": "eslint src",
-    "test": "echo \"To be implemented\"",
+    "test": "MONGODB_SERVICE_HOST=localhost MONGODB_DATABASE=notifications MONGODB_USER= MONGODB_PASSWORD= mocha --compilers js:babel-register -b test/spec/*.spec.js",


Ok this obviously needs to be changed. Would I be able to do npm run test -- MY_ENVS=something ?

or if you prefer you can add this package https://www.npmjs.com/package/better-npm-run that loads for you a .env file without touching source files

Excellent! Thank you

ghost · 2017-03-16T22:17:14Z

src/index.js

@@ -31,8 +21,7 @@ class Notification {
        self.io.sockets[user.id] = socket;
        self.sendUnread(user.id);


I believe we should take out this from here and let the user decide what actions should be taken when doing a check-in

ghost · 2017-03-16T22:18:14Z

src/index.js

+          .getAllUnread(userId)
+          .then((notifications) => {
+            const sentNotifications = notifications.map((notification) => {
+              const sentNotification = this.send('unread', notification);


Is it a good idea to have a hard-coded event like unread here? Or maybe pass it like a parameter

hard-coded should be fine, if it is used more than once then constant-it somewhere :p.

ghost · 2017-03-16T22:19:18Z

src/index.js

+          .getAllUnsent(userId)
+          .then((notifications) => {
+            const sentNotifications = notifications.map((notification) => {
+              const sentNotification = this.send('news', notification);


Same comment here for the news event. Also I think that's a wrong name, news sounds like paper news or something. Should be something like unsent

Unrelated, but you used events for this right? I think we should be consistent, if we are using promises we should use promises everywhere (even for tests although in there you should test for promises and callbacks to work)

You mean socket.io events?

If that's the only way to handle things in socket.io then ignore my comment :).

ghost · 2017-03-16T22:20:31Z

src/index.js

-        data.forEach((notification) => {
-          self.send('unread', notification);
-        });
+      const notificatonsSent = new Promise((resolve, reject) => {


I've added promises but I don't know if this will be the default approach. If I recall correctly we wanted to support both promises and callbacks

I'll read the whole code later because there are some things that I understand and I guess that's because of lack of context.

ghost · 2017-03-16T22:21:41Z

test/helpers.js

@@ -0,0 +1,20 @@
+const chance = require('chance').Chance(); // eslint-disable-line import/no-extraneous-dependencies
+
+const notificationTypes = ['web', 'mobile'];


Any ideas on any other type of notification?

ghost · 2017-03-16T22:22:00Z

test/notification.js

+
+const notificationDatabase = new MongoNotificationDatabase();
+databaseConnection.connect({
+  host: process.env.MONGODB_SERVICE_HOST,


Default values maybe?

I would say no if you ask me :)

Sounds good :)

jac1013

I think this task was related to unit tests (not integration test with a real database), Don't get me wrong, this is good either way we can discuss later about this in our meeting.

Edit: Seeing the issue is not specifying whether is integration tests or unit test, however this issue exist #4

jac1013 · 2017-03-16T23:07:10Z

src/index.js

+  constructor(database) {
+    this.notificationController = new NotificationController(database);
+
+    this.getAllUnread = this.notificationController.getAllUnread.bind(this.notificationController);


I don't understand why you need to do this, can you explain? Also it seems that is not being used.

Yeah it seems to me that is it not being used either. This had to do when I tried to use the class MongoNotificationDatabase that had a connection to mongo on the constructor. But that's not there anymore so I should remove this, thank you!

And in fact all of these methods seems unused to me

jac1013 · 2017-03-16T23:08:56Z

src/notifications/contracts/mongo-database-example.js

@@ -4,12 +4,16 @@ import NotificationDatabaseContract from './database';
 import Notification from './mongo-model-example';

 class MongoNotificationDatabase extends NotificationDatabaseContract {
+  constructor() {


Not needed, NotificationDatabaseContract works as an interface, it's only needed if you need to do something in the constructor (which you aren't) so adding the constructor with super doesn't make sense in here IMHO.

Yup you're right. Same comment as before, we were passing the connection parameters on the constructor of this class to open the connection to mongo right away. But that is not being used that way right know

jac1013

Hey @evasquez26 just a minor comment for you to remember, let's merge this as we agreed and do the changes for SQLite later.

jac1013 · 2017-03-29T17:50:20Z

package.json

    "compile": "babel src --out-dir dist"
  },
+  "betterScripts": {


Just as a reminder, we won't need this after you change everything to use SQLite @evasquez26

rkmax · 2017-03-29T18:53:02Z

src/index.js

+          this.notificationController
+            .update(notification._id, { sent: true }) //eslint-disable-line
+            .then(updatedNotification => resolve(updatedNotification))
+            .catch((err) => {


.catch(reject)

…stgreSQL Integrated better-npm-run, CR corrections Added .env to ignored files

jac1013 · 2017-03-31T20:05:09Z

@evasquez26 I fixed the conflicts here and I'm merging this branch because we need mocha library integration to continue with other tasks.

ghost requested review from EddSuarez, jac1013 and handmilkingsoftware March 16, 2017 22:11

ghost commented Mar 16, 2017

View reviewed changes

jac1013 requested changes Mar 16, 2017

View reviewed changes

ghost force-pushed the issue-#1-testing-is-missing branch from 2c2a568 to 4e7e6cc Compare March 16, 2017 23:51

jac1013 mentioned this pull request Mar 28, 2017

added Debug mode to the Module #19

Merged

jac1013 approved these changes Mar 29, 2017

View reviewed changes

rkmax reviewed Mar 29, 2017

View reviewed changes

Adding mocha to the project and creation of integration tests with po…

0130dfa

…stgreSQL Integrated better-npm-run, CR corrections Added .env to ignored files

jac1013 force-pushed the issue-#1-testing-is-missing branch from 54a30f6 to 0130dfa Compare March 31, 2017 20:04

jac1013 merged commit 78396e9 into master Mar 31, 2017

jac1013 deleted the issue-#1-testing-is-missing branch March 31, 2017 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing with mocha #20

Testing with mocha #20

ghost commented Mar 16, 2017 •

edited by ghost

Loading

ghost left a comment

ghost Mar 16, 2017

rkmax Mar 16, 2017

ghost Mar 16, 2017

ghost Mar 16, 2017

ghost Mar 16, 2017

jac1013 Mar 16, 2017 •

edited

Loading

ghost Mar 16, 2017

jac1013 Mar 16, 2017 •

edited

Loading

ghost Mar 16, 2017

jac1013 Mar 28, 2017

ghost Mar 16, 2017

jac1013 Mar 16, 2017

ghost Mar 16, 2017

ghost Mar 16, 2017

jac1013 Mar 16, 2017

ghost Mar 16, 2017

jac1013 left a comment •

edited

Loading

jac1013 Mar 16, 2017

ghost Mar 16, 2017

jac1013 Mar 16, 2017

ghost Mar 16, 2017

jac1013 left a comment

jac1013 Mar 29, 2017

rkmax Mar 29, 2017

jac1013 commented Mar 31, 2017

		@@ -31,8 +21,7 @@ class Notification {
		self.io.sockets[user.id] = socket;
		self.sendUnread(user.id);

		@@ -0,0 +1,20 @@
		const chance = require('chance').Chance(); // eslint-disable-line import/no-extraneous-dependencies

		const notificationTypes = ['web', 'mobile'];

Testing with mocha #20

Testing with mocha #20

Conversation

ghost commented Mar 16, 2017 • edited by ghost Loading

Story / Task

What this PR does?

Before

After

How to test / reproduce

Related PRs

TODOs

Migrations

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jac1013 Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jac1013 Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jac1013 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jac1013 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jac1013 commented Mar 31, 2017

ghost commented Mar 16, 2017 •

edited by ghost

Loading

jac1013 Mar 16, 2017 •

edited

Loading

jac1013 Mar 16, 2017 •

edited

Loading

jac1013 left a comment •

edited

Loading