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

💡 Fancy schmancy new test stuff #8206

Closed
wants to merge 1 commit into from

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Mar 21, 2017

Following on from the conversation in #8199, I had a quick play with some global stuff we can do to make writing & maintaining tests easier.

  • add a new test overrides file
  • register a special require function for server requires
  • turn should & sinon into globals
  • remove unused jshintrc file, update the used one!
  • update a few files as an example

Note: the grunt file change was indentation 🙈

Questions:

  • is this actually easier or is it just more confusing?
  • should we also expose a sandbox as a global or is that just dangerous?

- add a new test overrides file
- register a special require function for server requires
- turn should & sinon into globals
- remove unused jshintrc file, update the used one!
- update a few files as an example
@kirrg001
Copy link
Contributor

is this actually easier or is it just more confusing?

I am also not sure. But the good thing is, serverRequire is optional.

What we can also do is passing NODE_PATH as env option to the mochacli options.

 mochacli: {
    options: {             
      env: {
        NODE_PATH: path.join(__dirname, 'core','server') (or without server)
       }
    }
}

^ This is a working example, but not a full. I think we have to merge the original NODE_PATH.

Then you can do in a test:

require('api') or require('server/utils/url')

Another thing we could consider is making the testUtils globally available.
Maybe we can have a global ghost prefix e.g. ghost.testUtils

@ErisDS
Copy link
Member Author

ErisDS commented Mar 22, 2017

I deliberately decided to use the require-wrapping method with serverRequire (although perhaps requireFromServer makes more sense) instead of the modifying NODE_PATH method because I think the former is clearer.

If we modify the NODE_PATH on one line deep on our Gruntfile.js, I feel it will be very hard to understand why require('api') works, to not confuse it with a module or realise why it doesn't work serverside as well as in the tests. Meanwhile serverRequire('api') indicates this is not a normal require.

I think it's clearer, but not sure if it's clear enough.

Other options:

A. use require.main.require directly, this is a documented feature, and gives you the require from root, so require.main.require('core/server/api') instead of require('../../../../core/server/api') no less code, but clearer and doesn't require us to modify ../'s when files move.

B. create a global for serverPath that contains the base server path and do `require(serverPath + 'api'). Very simple, fiddly to type? Very clear though and still solves the problem of constantly modifying paths.


With regard to testUtils, yes, could also make these global but I didn't want to touch them as they are also a nebulous mess of things that need a lot of ❤️

@kirrg001
Copy link
Contributor

If we modify the NODE_PATH on one line deep on our Gruntfile.js, I feel it will be very hard to understand why require('api') works, to not confuse it with a module or realise why it doesn't work serverside as well as in the tests. Meanwhile serverRequire('api') indicates this is not a normal require.

Yeah i agree, maybe a bit too magical.

I like the A option.

With regard to testUtils, yes, could also make these global but I didn't want to touch them as they are also a nebulous mess of things that need a lot of ❤️

👍

@ErisDS
Copy link
Member Author

ErisDS commented Aug 1, 2017

Going to close this for now as its massively out of date. It's got some nice ideas we are experimenting with in other projects. We can maybe introduce the ideas one-by-one and get them merged later.

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.

2 participants