Standardize test style

Fix tests post-rebase

Author: Joshua Grosso

README.md

@@ -52,3 +52,141 @@ yarn
 
 **Tests are still a Work In Progress.**
 The tests rely on `mocha`, `chai`, `sinon`, and `sinon-chai`, and are currently a mix of system and unit tests.
+
+### Naming
+
+**TODO:** Split out system tests from unit tests and then revise the below guidlines.
+
+These guidelines are inspired by [Better Specs { rspec guidelines with ruby }](https://www.betterspecs.org). Given that these guidelines are, by necessity, quite subjective, consider them as beneficial for the consistency rather than the "correctness" that they provide.
+
+The linter should catch basic stylistic errors (e.g. prefer arrow functions unless `this` is needed).
+
+---
+
+When naming individual `it` blocks, the resulting test name should read like a sentence. For example: `it('returns false if the repo has no filters')`.
+
+Directly describe what the test should do. Use verbs like `returns`, `errors`, `responds`, `reads`, etc. rather than prefixing tests with `should`, suffixing them with `correctly`, etc.. For example:
+
+``` javascript
+/* Bad */
+it('should read globs from `.gitattributes` correctly', function () {
+  // ...
+});
+
+/* Good */
+it('reads globs from `.gitattributes`', function () {
+  // ...
+});
+```
+
+---
+
+If you have the temptation to start a test with `can`, try to restructure the test to use a nested `describe` instead (whose message starts with `when`). For example:
+
+``` javascript
+/* Bad */
+it('can initialize a non-LFS repo', function () {
+  // ...
+});
+
+/* Good */
+describe('when provided a non-LFS repo', () => {
+  it('initializes the repo', function () {
+    // ...
+  });
+});
+```
+
+---
+
+For tests whose behavior varies cross-platform, surround the tests with a `process.platform` test. Be sure to add tests for all platforms where behavior could vary, even if you cannot run them locally. Because CI will run on Windows, macOS, and Linux, this should not be an obstacle to thorough test coverage. For example:
+
+``` javascript
+/* Bad */
+describe('when on Windows', () => {
+  beforeEach(() => {
+    sinon.stub(process, 'platform').returns('win32');
+  });
+
+  it('runs the provided command', function () {
+    // ...
+  });
+});
+
+describe('when on macOS', () => {
+  beforeEach(() => {
+    sinon.stub(process, 'platform').returns('darwin');
+  });
+
+  it('runs the provided command', function () {
+    // ...
+  });
+});
+
+describe('when on Linux', () => {
+  beforeEach(() => {
+    sinon.stub(process, 'platform').returns('linux');
+  });
+
+  it('runs the provided command', function () {
+    // ...
+  });
+});
+
+/* Good */
+switch (process.platform) {
+  case 'win32':
+    it('runs the provided command', function () {
+      // ...
+    });
+    break;
+  case 'macOS':
+    it('runs the provided command', function () {
+      // ...
+    });
+    break;
+  case 'linux':
+    it('runs the provided command', function () {
+      // ...
+    });
+    break;
+}
+```
+
+---
+
+When referring to parameters, use `provided` rather than `passed-in`, `given`, etc. For example:
+
+``` javascript
+/* Bad */
+it('runs the passed-in command', function () {
+  // ...
+});
+
+/* Good */
+it('runs the provided command', function () {
+  // ...
+});
+```
+
+---
+
+Each implementation file should correspond to a similarly-named `<file name>.spec.js` test file in the same relative location. For example, `src/callbacks/check.js` should have a test file located in `test/tests/callbacks/check.spec.js`.
+
+The top-level `describe` should be named after the file. `describe`s for each function should be named after the function itself. On a related note, each function should have its own `describe` block, even if there is only `it` block inside it. In the case that a function does not have a pre-defined name (e.g. it's an anonymous function exported as the default), refer to it as `the default export`. For example:
+
+``` javascript
+/* For named exports: */
+describe('helpers', () => { // Named after the file name, `helpers.spec.js`
+  describe('hasLfsFilters', () => { // Named after the exported function name, `hasLfsFilters`
+    // ...
+  });
+});
+
+/* For default exports: */
+describe('check', () => {
+  describe('the default export', () => {
+    // ...
+  });
+});
+```

src/constants.js

@@ -5,7 +5,6 @@ export const LFS_FILTER_NAME = 'nodegit_lfs';
 export const regex = {
   LFS: /(?:git-lfs\/\s+)?(\d+)(?:.(\d+))?(?:.(\d+))?.*/,
   GIT: /(?:git\s+version\s+)(\d+\.){2}\d+/,
-  VERSION: /(\d+\.){2}\d+/,
   TRACK: /([a-zA-Z*.]+(?="))/g,
   SKIPPED_BYTES: /[\d]+\s+B\s+(?=skipped)/g,
   SKIPPED_FILES: /[\d]\s+(?=skipped)/g,

test/runner.js

@@ -77,8 +77,16 @@ beforeEach(function () {
 
   return NodeGitLFS.LFS.register()
     .then(() => setupEmptyTestRepo())
+    .then(() => NodeGitLFS.Repository.open(emptyRepoPath))
+    .then((emptyRepo) => {
+      this.emptyRepo = emptyRepo;
+    })
     .then(() => setupLfsTestRemote())
-    .then(() => setupLfsTestRepo());
+    .then(() => setupLfsTestRepo())
+    .then(() => NodeGitLFS.Repository.open(lfsTestRepoPath))
+    .then((lfsTestRepo) => {
+      this.lfsTestRepo = lfsTestRepo;
+    });
 });
 
 after(function () {

test/tests/callbacks/apply.spec.js

@@ -29,6 +29,7 @@ const track = (repo, globs) => {
 describe('apply', () => {
   beforeEach(function () {
     const {
+      lfsTestRepo,
       NodeGitLFS
     } = this;
 
@@ -68,15 +69,15 @@ describe('apply', () => {
     };
 
     this.trackTestFile = () =>
-      track(this.repo, [this.testFileName])
+      track(lfsTestRepo, [this.testFileName])
         .then(() => createDummyFile(
           path.join(lfsTestRepoPath, this.testFileName),
           testFileSize
         ))
         .then((contents) => {
           this.contents = contents;
         })
-        .then(() => this.commitFile(this.repo, this.testFileName, 'LFS filter tests'));
+        .then(() => this.commitFile(lfsTestRepo, this.testFileName, 'LFS filter tests'));
 
     this.verifyTestFileTracked = () =>
       fse.readFile(path.join(lfsTestRepoPath, this.testFileName), 'utf8')
@@ -87,37 +88,36 @@ describe('apply', () => {
       .then((pointer) => {
         expect(pointer).to.have.string(`size ${testFileSize}`);
       });
-
-    return NodeGitLFS.Repository.open(lfsTestRepoPath)
-      .then((repo) => {
-        this.repo = repo;
-      });
   });
 
-  it('Clean', function () {
-    const {
+  describe('clean', () => {
+    it('cleans LFS-tracked files', function () {
+      const {
       trackTestFile,
-      verifyTestFileTracked
+        verifyTestFileTracked
     } = this;
 
-    return trackTestFile()
-      .then(() => verifyTestFileTracked());
+      return trackTestFile()
+        .then(() => verifyTestFileTracked());
+    });
   });
 
-  it('Smudge', function () {
-    const {
-      NodeGitLFS,
-      repo,
-      testFileName,
-      trackTestFile,
-      verifyTestFileTracked
-    } = this;
+  describe('smudge', () => {
+    it('smudges LFS blobs', function () {
+      const {
+        NodeGitLFS,
+        lfsTestRepo,
+        testFileName,
+        trackTestFile,
+        verifyTestFileTracked
+      } = this;
 
-    return trackTestFile()
-      .then(() => createDummyFile(path.join(lfsTestRepoPath, testFileName), 5))
-      .then(() => NodeGitLFS.Checkout.head(repo, {
-        checkoutStrategy: NodeGitLFS.Checkout.STRATEGY.FORCE
-      }))
-      .then(() => verifyTestFileTracked());
-  }).timeout(10000);
+      return trackTestFile()
+        .then(() => createDummyFile(path.join(lfsTestRepoPath, testFileName), 5))
+        .then(() => NodeGitLFS.Checkout.head(lfsTestRepo, {
+          checkoutStrategy: NodeGitLFS.Checkout.STRATEGY.FORCE
+        }))
+        .then(() => verifyTestFileTracked());
+    }).timeout(10000);
+  });
 });

test/tests/callbacks/check.spec.js

@@ -5,77 +5,76 @@ import {
   Error
 } from 'nodegit';
 
-import {
-  lfsTestRepoPath
-} from '../../constants';
-
 import track from '../../../build/src/commands/track';
 import checkCallback from '../../../build/src/callbacks/check';
 
 describe('check', () => {
   beforeEach(function () {
     const {
-      NodeGitLFS
+      lfsTestRepo
     } = this;
 
     this.check = fileName => checkCallback({
       path: () => fileName,
-      repo: () => this.repo
+      repo: () => lfsTestRepo
     });
-
-    return NodeGitLFS.Repository.open(lfsTestRepoPath)
-      .then((repo) => {
-        this.repo = repo;
-      });
   });
 
-  it('returns `OK` for a file directly specified in .gitattributes', function () {
-    const {
-      check,
-      repo
-    } = this;
+  describe('the default export', () => {
+    describe('when the provided file is directly specified in `.gitattributes`', () => {
+      it('returns `OK` for the provided file', function () {
+        const {
+          check,
+          lfsTestRepo
+        } = this;
 
-    return track(repo, ['test.txt'])
-      .then(() => check('test.txt'))
-      .then((result) => {
-        expect(result).to.equal(Error.CODE.OK);
+        return track(lfsTestRepo, ['test.txt'])
+          .then(() => check('test.txt'))
+          .then((result) => {
+            expect(result).to.equal(Error.CODE.OK);
+          });
       });
-  });
+    });
 
-  it('returns `OK` for a file included in a glob in .gitattributes', function () {
-    const {
-      check,
-      repo
-    } = this;
+    describe('when the provided file is included in a glob in `.gitattributes`', () => {
+      it('returns `OK` for the provided file', function () {
+        const {
+           check,
+          lfsTestRepo
+         } = this;
 
-    return track(repo, ['*.txt'])
-      .then(() => check('test.txt'))
-      .then((result) => {
-        expect(result).to.equal(Error.CODE.OK);
+        return track(lfsTestRepo, ['*.txt'])
+          .then(() => check('test.txt'))
+          .then((result) => {
+            expect(result).to.equal(Error.CODE.OK);
+          });
       });
-  });
+    });
 
-  it('returns `PASSTHROUGH` for a file not included in .gitattributes', function () {
-    const {
-      check,
-      repo
-    } = this;
+    describe('when the provided file is not included in `.gitattributes`', () => {
+      it('returns `PASSTHROUGH`', function () {
+        const {
+          check,
+          lfsTestRepo
+        } = this;
 
-    return track(repo, ['other.txt'])
-      .then(() => check('test.txt'))
-      .then((result) => {
-        expect(result).to.equal(Error.CODE.PASSTHROUGH);
+        return track(lfsTestRepo, ['other.txt'])
+          .then(() => check('test.txt'))
+          .then((result) => {
+            expect(result).to.equal(Error.CODE.PASSTHROUGH);
+          });
       });
-  });
+    });
 
-  it('returns `PASSTHROUGH` on error', function () {
-    const {
-      check
-    } = this;
+    it('returns `PASSTHROUGH` on error', function () {
+      const {
+        check
+      } = this;
 
-    return check('test.txt')
-      .then((result) => {
-        expect(result).to.equal(Error.CODE.PASSTHROUGH);
-      });
+      return check('test.txt')
+        .then((result) => {
+          expect(result).to.equal(Error.CODE.PASSTHROUGH);
+        });
+    });
   });
 });

test/tests/callbacks/initialize.spec.js

@@ -7,8 +7,10 @@ import {
 
 import initialize from '../../../build/src/callbacks/initialize';
 
-describe('Initialize', () => {
-  it('returns `OK`', () => {
-    expect(initialize()).to.equal(Error.CODE.OK);
+describe('initialize', () => {
+  describe('the default export', () => {
+    it('returns `OK`', () => {
+      expect(initialize()).to.equal(Error.CODE.OK);
+    });
   });
 });