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

💄 🐷 Test consistency #8199

Merged
merged 1 commit into from
Mar 21, 2017
Merged

💄 🐷 Test consistency #8199

merged 1 commit into from
Mar 21, 2017

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Mar 20, 2017

  • change out should.equal for // jshint ignore:line
  • ensure should is the first require in every test, and ALWAYS require
  • make sinon the second require, and sandbox the last thing
  • ALWAYS use sandbox, futureproofs tests against contributors who don't know it
  • change require formatting

Most of these changes just make it easy to grok that the test structure is correct. I keep finding files that didn't have should! I'm sure it'll go wrong again, but for now 👍

@@ -0,0 +1,59 @@
<!doctype html>

This comment was marked as abuse.

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

👍

What we also can do is, we add a script in core/test, which will require all the basic test modules we need (should, sinon) and assign as globals. Then we tell mocha to load this script before the tests are executed, here https://github.com/TryGhost/Ghost/blob/master/Gruntfile.js#L147

Each test will have access to the test modules by default.

- change out should.equal for // jshint ignore:line
- ensure should is the first require in every test, and ALWAYS require
- make sinon the second require, and sandbox the last thing
- ALWAYS use sandbox, futureproofs tests against contributors who don't know it
- change require formatting
@ErisDS
Copy link
Member Author

ErisDS commented Mar 20, 2017

Yeah I'd also like to setup a global called serverRequire to reduce the ../../../ 👍

@ErisDS
Copy link
Member Author

ErisDS commented Mar 21, 2017

To be clear, I'm not doing those things in this PR! 😁

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

yeah i know 👍 Just waited for the admin view update.

@kirrg001 kirrg001 merged commit 47e0090 into TryGhost:master Mar 21, 2017
@ErisDS ErisDS deleted the test-cleanup branch October 19, 2017 14:35
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.

3 participants