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

Split up test file #185

Merged
merged 1 commit into from
Apr 22, 2016
Merged

Conversation

svozza
Copy link
Member

@svozza svozza commented Apr 20, 2016

I'm not going to get this finished tonight but I figure that we may as well start the review now given there's so much in here. I've only got the List, String and Number tests left to do. I'll change the name of the issue when I'm finished too.

@davidchambers
Copy link
Member

Thanks! This'll be a nice change. :)

@svozza svozza force-pushed the split-test-files branch from 90cd5d5 to e2e944f Compare April 20, 2016 22:36
eq(S.A(S.inc)(1), 2);
});

});
Copy link
Member

Choose a reason for hiding this comment

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

Regarding spacing:

  • Let's include one empty line after the 'use strict' pragma.

  • Let's include one empty line between third-party imports and local imports.

  • Let's include two empty lines after the last import.

  • Let's end each file with exactly one trailing \n. This script may be useful:

    #!/usr/bin/env bash
    set -e
    
    # Update the specified files so that each nonempty file ends with
    # exactly one "\n".
    # http://unix.stackexchange.com/q/81685
    for file ; do
      data="$(<"$file")"
      [[ $data == '' ]] || printf '%s\n' "$data" >"$file"
    done

Bring it all together, we'd get:

'use strict';

var R = require('ramda');

var S = require('..');
var eq = require('./shared/utils').eq;


describe('A', function() {

  it('is a binary function', function() {
    eq(typeof S.A, 'function');
    eq(S.A.length, 2);
  });

  it('A(f, x) is equivalent to f(x)', function() {
    eq(S.A(S.inc, 1), 2);
    eq(R.map(S.A(R.__, 100), [S.inc, Math.sqrt]), [101, 10]);
  });

  it('is curried', function() {
    eq(S.A(S.inc).length, 1);
    eq(S.A(S.inc)(1), 2);
  });

});

Copy link
Member Author

Choose a reason for hiding this comment

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

You're a day late with this advice! ;)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry! You could write a program to update the headers. I don't know whether it'd save time, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hehe. No bother, I knew what I was signing up for when I offered to do this. Will be a finicky one to get right!

@svozza svozza force-pushed the split-test-files branch 7 times, most recently from 8085944 to 4e3d120 Compare April 21, 2016 21:12
@svozza svozza changed the title Split up test file (WIP) Split up test file Apr 21, 2016
@svozza
Copy link
Member Author

svozza commented Apr 21, 2016

All done but for some reason the build is taking ages.

var assert = require('assert');
var R = require('ramda');

var S = require('../..');
Copy link
Member

Choose a reason for hiding this comment

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

'use strict';

var assert = require('assert');     // } group for standard library imports

var R = require('ramda');           // } group for third-party imports

var S = require('../..');           // } group for local imports

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, I was sure I fixed these already. I even ran the extra line script just before I did my last push.

@svozza svozza force-pushed the split-test-files branch 2 times, most recently from ae073c4 to 02b7cc3 Compare April 21, 2016 22:28
@svozza
Copy link
Member Author

svozza commented Apr 21, 2016

⚡ x \n

@davidchambers
Copy link
Member

LGTM! My last question is whether we test/shared is necessary. Do you envisage other files living in the directory one day? If not, I suggest moving test/shared/utils.js to test/shared.js.

@svozza svozza force-pushed the split-test-files branch from 02b7cc3 to 5590e7c Compare April 22, 2016 05:34
@svozza
Copy link
Member Author

svozza commented Apr 22, 2016

Yeah, initially I thought there would be more shared functions and that we might want to split them into different files but there were much less than I expected. ⚡

$(JSHINT) -- index.js test/index.js
$(JSCS) -- index.js test/index.js
$(JSHINT) -- index.js test/**/*.js
$(JSCS) -- index.js test/**/*.js
Copy link
Member

Choose a reason for hiding this comment

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

Should this be test/*.js now? make lint is failing in CircleCI.

@svozza
Copy link
Member Author

svozza commented Apr 22, 2016

Interesting. I've been run npm test all along thinking that it was running the linter but it's not.

@svozza svozza force-pushed the split-test-files branch from 5590e7c to 95dd93b Compare April 22, 2016 06:16
@svozza
Copy link
Member Author

svozza commented Apr 22, 2016

I've added -W053: true to jshintrc because I don't want to add the annotation into 40+ files when we're going to be moving to ESLint soon anyway.

@davidchambers
Copy link
Member

🌳 Thanks so much, Stefano! This was a big undertaking. :)

@svozza
Copy link
Member Author

svozza commented Apr 22, 2016

No probs! 😄

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