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

Init Tests #623

Merged
merged 3 commits into from Jan 17, 2017
Merged

Init Tests #623

merged 3 commits into from Jan 17, 2017

Conversation

roramirez
Copy link
Contributor

No description provided.

    This patch propouse use the mocha for testing MagicMirror.
@MichMich
Copy link
Collaborator

Does this mean you're going to work on a full test suite?

@roramirez
Copy link
Contributor Author

Hi @MichMich

I've interest a work in a test suite for MagicMirror. I think is somethings we need.

I can do develop somes tests. This patch propose the use mocha for the test suite.

I would like to hear comments and if we can work a list set of test to do.

@MichMich
Copy link
Collaborator

Honestly, I don't have much experience with writing test. This would be a nice project to learn more.

@MichMich MichMich merged commit 2d8d258 into MagicMirrorOrg:develop Jan 17, 2017
@roramirez
Copy link
Contributor Author

We can invite to users in forums. I think is really important define the test list for will do. The test suite is a little big work, isn't something we can do it in one day or week.

@qistoph
Copy link
Contributor

qistoph commented Jan 18, 2017

One of the changes in 3a8d72d introduces an error in the client.

js/module.js:419

Uncaught ReferenceError: exports is not defined at module.js:419

@roramirez
Copy link
Contributor Author

I see. Let me check how prevent this. If you have any suggest or aprouch is welcome :)

Copy link
Contributor

@nhubbard nhubbard left a comment

Choose a reason for hiding this comment

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

Check my comments on how to fix this.

@@ -415,3 +415,7 @@ Module.register = function (name, moduleDefinition) {
Log.log("Module registered: " + name);
Module.definitions[name] = moduleDefinition;
};

exports._test = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you would do this instead:

module.exports = {
    cmpVersions: cmpVersions
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't work :(. I sent a new Pull Request with a hotfix. Can you review @nhubbard and @qistoph this?

Also, I add a post for create a set list of test for the suite.
https://forum.magicmirror.builders/topic/1456/test-suite-for-magicmirror
I would really like have your comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR #633 removed the error for me and npm test works. Seems like a good solution to me.

Also noticed a minor textual improvement in tests/functions/compare-version.js:
"Test function ... into ..." could better be "Test function ... in ...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I pushed the textual change.

roramirez added a commit to roramirez/MagicMirror that referenced this pull request Jan 20, 2017
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.

None yet

4 participants