Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/1502 #1691

Merged
merged 29 commits into from
Aug 11, 2017
Merged

Issue/1502 #1691

merged 29 commits into from
Aug 11, 2017

Conversation

canstudios-louisem
Copy link
Contributor

  • Bring in Kineo tests
  • Tests now always start with a clean DB
  • Upgrade should as Kineo tests needed it
  • Fix tests broken from changes and should upgrade

#1502

@canstudios-louisem canstudios-louisem added this to the 0.4.0 Refactor milestone Aug 2, 2017
}

return callback(new Error('No matching tenant record found'));
return self.updateTenant({ '_id': results[0]._id }, { _isDeleted: true }, callback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

results is no longer defined?

@taylortom
Copy link
Member

Tests should be working smoothly now

@canstudios-nicolaw
Copy link
Contributor

The tests are all passing for me, I have noticed I get the following output when I run them

error: [07 Aug 2017 10:56:45 +01:00] test foo=bar

I just wanted to check this is something we are expecting/aware of?

@taylortom
Copy link
Member

taylortom commented Aug 8, 2017

@canstudios-nicolaw that's because of this test. We can set up the logger to mute all messages during the tests if we want? (log level is set here).

@canstudios-nicolaw
Copy link
Contributor

Ok I am happy as long as we understand where it is coming from :) It feels a bit dangerous to mute all messages in case we miss something in future.

Copy link
Member

@lc-thomasberger lc-thomasberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we can remove make it would be easier to run the tests on Windows.
At least we should point to the workaround with running the test's with node_modules/mocha/bin/mocha

@taylortom
Copy link
Member

Have updated the test script to use the grunt 'test' task, thus removing the need for a makefile.

Copy link
Contributor Author

@canstudios-louisem canstudios-louisem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im happy you have updated the package.json but stuff like that should really be in a sperate pr.

@@ -357,7 +349,6 @@ module.exports = function(grunt) {
}
});

grunt.registerTask('test', ['mochaTest'/*, 'casperjs'*/]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the package.json's test command as it curently does make test not grunt test. Im not sure how much it matters really at Can we all use php storm's mocha test runner to run unit tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest package.json should use grunt test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice for some reason i missed you had changed it.

@taylortom
Copy link
Member

@canstudios-louisem you're right, I just knew if I waited to do a separate PR, I'd likely forget 😅

@canstudios-louisem
Copy link
Contributor Author

that's why i added a reminder months a go :) #1649

Copy link
Contributor

@canstudios-nicolaw canstudios-nicolaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving since more changes have been made

@taylortom taylortom merged commit f55057d into develop Aug 11, 2017
@taylortom taylortom deleted the issue/1502 branch August 11, 2017 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants