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

ROADMAP.md: Add test generator to backlog. #2499

Closed
wants to merge 1 commit into from
Closed

ROADMAP.md: Add test generator to backlog. #2499

wants to merge 1 commit into from

Conversation

jwahyoung
Copy link

Add test generator to project backlog. Sorry if this is in the wrong repo - if it should be in a sails subrepository, let me know!

@jwahyoung
Copy link
Author

Implementation details:

I believe that sails-generate-test should be built compositionally; for examplesails generate test User:Model would call a test-model generator in it's targets configuration. While a default could be provided, this would allow for overriding the test generators via .sailsrc. Generators could be provided for sails-generate-test-model, sails-generate-test-controller, and sails-generate-test-policy.

If possible, each generator type could read the specified model/controller/whatever and generate tests for each method in that module. This may be possible if the sails global is available to the generator, by checking sails.models, or the equivalent entity type. I'm not really sure about this part yet, but I think it'd be nice to automagically generate test stubs for each method in the existing entity.

@mikermcneil
Copy link
Member

@jedd-ahyoung thanks! I rebased just now because I'd been dinking with the file, but it's in there :)

@mikermcneil
Copy link
Member

@jedd-ahyoung my reactions below:

I believe that sails-generate-test should be built compositionally

👍

While a default could be provided, this would allow for overriding the test generators via .sailsrc.

👍 100% w/ you

Generators could be provided for sails-generate-test-model, sails-generate-test-controller, and sails-generate-test-policy.

Perfect- let's start with controllers/actions though?

If possible, each generator type could read the specified model/controller/whatever and generate tests for each method in that module

tricky for anything but controllers currently (b/c javascript man- but check out http://node-machine.org/ for my proposed solution)

@jwahyoung
Copy link
Author

Yeah, I'd like to help with this. I started on something a while back, here: https://github.com/jedd-ahyoung/sails-generate-test

It's very rudimentary, and can't really be used yet, but I think it's a start, perhaps. What do you think?

@mikermcneil
Copy link
Member

@jedd-ahyoung Apologies for the very late response to your excellent suggestion, Jedd.

I'm in the process of cleaning up our roadmap, and so we have something on the books, I'd like to propose a watered down first pass here (we can always follow up once this is in place):

Use Case

Historically, testing with Sails has not been very opinionated-- it's largely been left up to the user. But after having written many hundreds of tests for Sails APIs, it became clear that a default setup for testing would make a great addition to the Sails CLI by default. Currently, initially setting up tests in a Sails app requires either some creative planning (if you've never done it), or repetitive copy & pasting (if you have). Rather than prescribing a particular test generator, let's be opinionated about the default test framework and go from there.

Usage Summary

$ sails new foo

When a new Sails project is created, a test/ folder is also created:

./foo
├── api/
├── assets/
├── ...
├── test/
│  ├── integration/
│  │  └── .gitkeep
│  ├── fixtures/
│  │  └── .gitkeep
│  ├── bootstrap.test.js
│  └── mocha.opts
└── views/

The test folder contains:

  • a sample mocha.opts file (e.g. configures a timeout of 5000ms)
  • the subfolders and .gitkeep files shown above
  • a setup/teardown test helper function named bootstrap.test.js as defined here

Then, out of the box, running npm test should work (first applying mocha.opts, running the bootstrap.test.js file, then running all tests nested inside the integration/ directory or any of its subdirectories).

Impact to Sails core

Documentation

  • need to add relevant pages to the "Anatomy" docs here
  • need to update package.json in the Anatomy docs to reflect the changes from adding this feature (the npm test script, as well as the devDependency on mocha)

Misc.

  • Note that this proposal does include the ability to specify e.g. a command-line flag to use a different test runner out of the box.
  • Also note that this proposal specifically chooses to not include the ability to generate tests themselves-- just the initial setup. That part of the equation differs a lot between developers, teams, and apps, and it is not clear what an opinionated version of sails generate test would look like yet (would be great as a separate proposal)

If anyone reading this would like to make refinements, clarifications, or corrections to the above (or has additional related ideas/comments), please reply in this pull request. Also please let me know about any confusing statements or typos. Thanks!

@luislobo
Copy link
Contributor

I've been thinking a lot about adding this feature since months ago. As always, time is short.
For some of our projects, we have implemented the tests as new sails tasks.
/config/mochaTest.js

/**
 * Mocha Test
 * 
 */
module.exports = function(grunt) {

  grunt.config.set('mochaTest', {
    test: {
      options: {
        reporter: 'list'
      },
      src: ['test/**/*.spec.js']
    },
    testdev: {
      options: {
        reporter: 'spec',
        bail: true
      },
      src: ['test/**/*.spec.js']
    }
  });


  grunt.loadNpmTasks('grunt-mocha-test');
};

and

module.exports = function (grunt) {
    grunt.registerTask('test', ['mochaTest']);
  grunt.registerTask('testdev', ['mochaTest:testdev']);
};

For other projects we just changed our Gruntfile.js to look something like this:

/*global module:false*/
module.exports = function (grunt) {

  // Project configuration.
  grunt.initConfig({
    // Task configuration.
    jshint: {
      // http://jshint.com/docs/options
      options: {
        // See http://jshint.com/docs/ for more details
        "maxerr": 50, // {int} Maximum error before stopping
        // Enforcing
        "bitwise": true, // true: Prohibit bitwise operators (&, |, ^, etc.)
        "camelcase": false, // true: Identifiers must be in camelCase
        "curly": true, // true: Require {} for every new block or scope
        "eqeqeq": true, // true: Require triple equals (===) for comparison
        "forin": false, // true: Require filtering for..in loops with obj.hasOwnProperty()
        "freeze": true, // true: prohibits overwriting prototypes of native objects such as Array, Date etc.
        "indent": 2, // {int} Number of spaces to use for indentation
        "latedef": true, // true: Require variables/functions to be defined before being used
        "newcap": false, // true: Require capitalization of all constructor functions e.g. `new F()`
        "noarg": true, // true: Prohibit use of `arguments.caller` and `arguments.callee`
        "noempty": true, // true: Prohibit use of empty blocks
        "nonbsp": true, // true: Prohibit "non-breaking whitespace" characters.
        "nonew": true, // true: Prohibit use of constructors for side-effects (without assignment)
        "plusplus": false, // true: Prohibit use of `++` and `--`
        "quotmark": false, // Quotation mark consistency:
        //   false    : do nothing (default)
        //   true     : ensure whatever is used is consistent
        //   "single" : require single quotes
        //   "double" : require double quotes
        "undef": true, // true: Require all non-global variables to be declared (prevents global leaks)
        "unused": true, // Unused variables:
        //   true     : all variables, last function parameter
        //   "vars"   : all variables only
        //   "strict" : all variables, all function parameters
        "strict": false, // true: Requires all functions run in ES5 Strict Mode
        "maxparams": false, // {int} Max number of formal params allowed per function
        "maxdepth": false, // {int} Max depth of nested blocks (within functions)
        "maxstatements": false, // {int} Max number statements per function
        "maxcomplexity": false, // {int} Max cyclomatic complexity per function
        "maxlen": false, // {int} Max number of characters per line
        // Relaxing
        "asi": false, // true: Tolerate Automatic Semicolon Insertion (no semicolons)
        "boss": false, // true: Tolerate assignments where comparisons would be expected
        "eqnull": false, // true: Tolerate use of `== null`
        "funcscope": false, // true: Tolerate defining variables inside control statements
        "validthis": false, // true: Tolerate using this in a non-constructor function
        //reporter: require('jshint-stylish'),
        force: false
      },
      gruntfile: {
        globals: {
          require: false
        },
        src: 'Gruntfile.js'
      },
      lib_test: {
        options: {
          node: true,
          "globals": {
            "sails": false,
            "Node": false,
            "describe": false,
            "xdescribe": false,
            "ddescribe": false,
            "it": false,
            "xit": false,
            "iit": false,
            "beforeEach": false,
            "afterEach": false,
            "expect": false,
            "pending": false,
            "should": false,
            "spyOn": false
          }
          //reporter: require('jshint-stylish')
        },
        src: ['lib/**/*.js', 'test/**/*.spec.js']
      }
    },
    mochaTest: {
      test: {
        options: {
          reporter: 'spec',
          timeout: 5000
        },
        src: ['test/**/*.spec.js']
      }
    },
    watch: {
      gruntfile: {
        files: '<%= jshint.gruntfile.src %>',
        tasks: ['jshint:gruntfile']
      },
      lib_test: {
        files: '<%= jshint.lib_test.src %>',
        tasks: ['jshint:lib_test', 'mochaTest']
      },
      default: {
        files: 'lib/**/*.js',
        tasks: ['jshint']
      }
    }
  });

  // These plugins provide necessary tasks.
  grunt.loadNpmTasks('grunt-mocha-test');
  grunt.loadNpmTasks('grunt-contrib-jshint');
  grunt.loadNpmTasks('grunt-contrib-watch');

  //testing
  grunt.registerTask('test', ['mochaTest']);
  // Default task.
  grunt.registerTask('default', ['jshint', 'test']);

};

Would you prefer this approach or just add the specific commands inside package.json?

I would definitely implement this feature.

@luislobo
Copy link
Contributor

btw, I really would like the idea of including jshint too.

@mikermcneil
Copy link
Member

@luislobo thanks for the insight on how you guys do it, Luis. That's a cool solution and I think a lot of folks will benefit from seeing it. I'd like to get the most basic implementation as specced above completed first since it's the lowest common denominator for most teams' testing strategies. In general, I'm not sure if we can commit to maintaining more Grunt tasks as part of the default install of Sails, but I think that's ok- seems like what you're suggesting could be implemented in a pretty straightforward way as a generator (although a Grunt task might probably be easier to install).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants