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

Versions returned in the wrong order #26

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
"mocha": "latest",
"chai": "latest",
"debug": "~0.7.2",
"ini": "~1.1.0"
"ini": "~1.1.0",
"proxyquire": "~0.5.1",
"sinon": "~1.7.3"
},
"optionalDependencies": {},
"engines": {
Expand Down
23 changes: 15 additions & 8 deletions server.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,10 @@ server.get('/:name', function (req, res) {
var meta = data.packageMeta(packageName);
if (!meta) return notFound(res);

var versions = data.whichVersions(packageName).sort();
var versions = getPackageVersions(packageName);
var versionsData = {};
var times = {};
versions.forEach(function(v) {
versions.all.forEach(function(v) {
versionsData[v] = meta.versions[v].data;
times[v] = meta.versions[v].time;
});
Expand All @@ -173,7 +173,7 @@ server.get('/:name', function (req, res) {
name: meta.name,
description: meta.description,
'dist-tags': {
latest: versions[versions.length-1]
latest: versions.latest
},
versions: versionsData,
maintainers: [],
Expand Down Expand Up @@ -210,11 +210,10 @@ function listAction(req, res) {
res.json(200, result);

function getPackageInfo(packageName) {
var versions = data.whichVersions(packageName).sort();
var versions = getPackageVersions(packageName);
var meta = data.packageMeta(packageName);
var lastVersion = versions[versions.length-1];
var versionsData = {};
versions.forEach(function(v) {
versions.all.forEach(function(v) {
versionsData[v] = 'latest';
});

Expand All @@ -223,19 +222,27 @@ function listAction(req, res) {
name: meta.name,
description: meta.description,
'dist-tags': {
latest: lastVersion
latest: versions.latest
},
versions: versionsData,
maintainers: [],
author: meta.author,
repository: meta.repository,
time: {
modified: meta.versions[lastVersion].time
modified: meta.versions[versions.latest].time
}
};
}
}

function getPackageVersions(packageName) {
Copy link
Author

Choose a reason for hiding this comment

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

I just DRYed two birds with one stone by adding this. Hopefully that is an acceptable alternative to altering data.js.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, your solution is fine.

var versions = data.whichVersions(packageName).sort(semver.compare);
return {
all: versions,
latest: semver.maxSatisfying(versions, '*')
};
}

server.put('/:name/-/:filename/-rev/:rev', function (req, res) {
var filename = req.params.filename;
var rand = Math.floor(Math.random()*4294967296).toString(36);
Expand Down
105 changes: 105 additions & 0 deletions test/server.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
'use strict';

var proxyquire = require('proxyquire'),
expect = require('chai').expect,
sinon = require('sinon'),
semver = require('semver');

var VERBS = ['get', 'post', 'put', 'del'];

describe('reggie npm server (unit)', function () {
var reggieServer,
req = {},
res = {},
routes = {},
restify = {},
data = function Data() {};

var versions = [
'1.0.0',
'1.0.1',
'1.0.2-2',
'1.0.2-10'
];
data.prototype.packageMeta = function () {
return {
versions: versions.reduce(function (verObj, currVer) {
verObj[currVer] = {data: {}, time: {}};
return verObj;
}, {})
};
};
data.prototype.whichVersions = function () {
return versions;
};

beforeEach(function () {
//Store route functions in a hash
VERBS.forEach(function (verb) { routes[verb] = {}; });

//mock restify
restify.createServer = function() {
var serverMock = {
'use': function () {},
'listen': function () {}
};
VERBS.forEach(function (verb) {
serverMock[verb] = function (route, cb) {
routes[verb][route] = cb;
};
});
return serverMock;
};

data.prototype.init = function () {};

reggieServer = proxyquire('../server', {
'restify': restify,
'./lib/data': data
});

req.params = { name: 'test' };
res.json = sinon.spy();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are already tests for npm server in test/npm-protocol.js.

I believe that integration tests are better for testing implementation of npm protocol, because we want to ensure reggie fulfils whatever npm client expects from the server.

Have you considered that? What are your arguments for going with unit-tests & mocking instead? Let's discuss this first.

Copy link
Author

Choose a reason for hiding this comment

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

These are basically regression tests for a specific bug. If the tests were E2E, a change unrelated to the bug could cause the test to fail even though the bug hasn't actually resurfaced.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough.

Now there are few things to improve in your unit-tests:

  1. Test naming - use present tense in it descriptions, existing tests are using that convention. E.g.
it('chooses latest by semver order', // etc
  1. File naming
  • Existing tests (i.e. test/npm-protocol.js) do not add .test into file names.
  • It is not clear which files contain pure unit-tests and which contain integration tests.

We can nest tests in test/unit/server.js and test/integration/npm-protocol.js. Can you come up with a better alternative?

(cc: @paolodm - the same problem of unit vs. integration tests applies to your pull request #22)

  1. Test data naming - please don't use test as package name, it is rather ambiguous name. test-package or even a-package-name is much better (the latter is communicating the fact that any package name will work).

  2. The way how your tests are structured makes it difficult to add new tests using different data (packages) in the future. The reason is that your tests rely on global/shared settings. It is ok to setup Server/Data mocks without specific data/behaviour in the beforeEach hook. However, each test should define the test data and mocked behaviour on its own. Example:

it('Should choose latest by semver order', function () {
  givenPackageWithVersions('test-package', ['1.0.2-2', '1.0.2-10']);
  req.name = 'test-package';
  routes.get['/:name'](req, res);
  expect(res.json.firstCall.args[1]['dist-tags'].latest).to.equal('1.0.2-10');
});
  1. This piece of code requires some effort to understand what it does, plus it is repeated many times:
res.json.firstCall.args[1]

Please extract it into a helper function. Alternatively, set up sinon to save the json result to a variable and possibly assert that it was called only once.


describe('/:name', function () {
beforeEach(function () {
routes.get['/:name'](req, res);
});

it('Should choose latest by semver order', function () {
expect(res.json.firstCall.args[1]['dist-tags'].latest).to.equal('1.0.2-10');
});

it('Should "sort" by semver order', function () {
// Yes, this is depending on an implementation detail of Object.keys
// Yes, that violates the API contract of Object and is bad
// Yes, this is how the npm client actually works
var resultVersions;
resultVersions = Object.keys(res.json.firstCall.args[1].versions);
expect(resultVersions).to.deep.equal(versions.sort(semver.compare));
});
});

describe('/-/all', function () {
beforeEach(function () {
data.prototype.getAllPackageNames = function () {
return ['test'];
};
routes.get['/-/all'](req, res);
});

it('Should choose "latest" by semver order', function () {
expect(res.json.firstCall.args[1].test['dist-tags'].latest).to.equal('1.0.2-10');
});

it('Should "sort" by semver order', function () {
// Yes, this is depending on an implementation detail of Object.keys
// Yes, that violates the API contract of Object and is bad
// Yes, this is how the npm client actually works
var resultVersions;
resultVersions = Object.keys(res.json.firstCall.args[1].test.versions);
expect(resultVersions).to.deep.equal(versions.sort(semver.compare));
});
});
});