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

various fixes and tests for [githubpipenv] #7194

Merged
merged 1 commit into from
Oct 28, 2021
Merged
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
26 changes: 15 additions & 11 deletions services/pipenv-helpers.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,19 @@
import Joi from 'joi'
import { InvalidParameter } from './index.js'

const isDependency = Joi.alternatives(
Joi.object({
version: Joi.string().required(),
}).required(),
Joi.object({
ref: Joi.string().required(),
}).required()
)
const isDependency = Joi.object({
version: Joi.string(),
ref: Joi.string(),
}).required()

const isLockfile = Joi.object({
_meta: Joi.object({
requires: Joi.object({
python_version: Joi.string(),
}).required(),
}).required(),
default: Joi.object().pattern(Joi.string().required(), isDependency),
develop: Joi.object().pattern(Joi.string().required(), isDependency),
default: Joi.object().pattern(Joi.string(), isDependency),
develop: Joi.object().pattern(Joi.string(), isDependency),
}).required()

function getDependencyVersion({
Expand Down Expand Up @@ -45,8 +41,16 @@ function getDependencyVersion({
if (version) {
// Strip the `==` which is always present.
return { version: version.replace('==', '') }
} else if (ref) {
if (ref.length === 40) {
// assume it is a commit hash
return { ref: ref.substring(0, 7) }
}
return { ref } // tag
} else {
return { ref: ref.substring(1, 8) }
Copy link
Member

Choose a reason for hiding this comment

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

i realize now I'm even more confused by what we were doing before. any idea why we were substringing from this range before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure I probably should have explained this more as there were 2 issues here:

  • 1 is that we were assuming ref is always a commit hash, whereas it can be a commit hash or a tag (or a branch name actually, now I think of it)
  • The other is that if it is a commit hash, we want the first 7 chars, whereas we were actually taking characters 2 though 8 which was passing the service test because we're just doing a picture check
    const isShortSha = Joi.string().regex(/[0-9a-f]{7}/)
    but was not actually correct.

throw new InvalidParameter({
prettyMessage: `No version or ref for ${wantedDependency}`,
})
}
}

Expand Down
131 changes: 131 additions & 0 deletions services/pipenv-helpers.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import { expect } from 'chai'
import { getDependencyVersion } from './pipenv-helpers.js'
import { InvalidParameter } from './index.js'

describe('getDependencyVersion', function () {
// loosely based on https://github.com/pypa/pipfile#pipfilelock
const packages = {
chardet: {
hashes: [
'sha256:fc323ffcaeaed0e0a02bf4d117757b98aed530d9ed4531e3e15460124c106691',
'sha256:84ab92ed1c4d4f16916e05906b6b75a6c0fb5db821cc65e70cbd64a3e2a5eaae',
],
version: '==3.0.4',
},
django: {
editable: true,
git: 'https://github.com/django/django.git',
ref: '1.11.4001',
},
'django-cms': {
file: 'https://github.com/divio/django-cms/archive/release/3.4.x.zip',
},
'discordlists-py': {
git: 'https://github.com/HexCodeFFF/discordlists.py',
ref: '2df5a2b62144b49774728efa8267d909a8a9787f',
},
}

it('throws if dependency not found in (in default object with data)', function () {
expect(() =>
getDependencyVersion({
wantedDependency: 'requests',
lockfileData: {
default: packages,
develop: {},
},
})
)
.to.throw(InvalidParameter)
.with.property('prettyMessage', 'default dependency not found')
})

it('throws if dependency not found in (in empty dev object)', function () {
expect(() =>
getDependencyVersion({
kind: 'dev',
wantedDependency: 'requests',
lockfileData: {
default: packages,
develop: {},
},
})
)
.to.throw(InvalidParameter)
.with.property('prettyMessage', 'dev dependency not found')
})

it('tolerates missing keys', function () {
expect(
getDependencyVersion({
wantedDependency: 'chardet',
lockfileData: {
default: packages,
},
})
).to.deep.equal({ version: '3.0.4' })
})

it('finds package in develop object', function () {
expect(
getDependencyVersion({
kind: 'dev',
wantedDependency: 'chardet',
lockfileData: {
default: {},
develop: packages,
},
})
).to.deep.equal({ version: '3.0.4' })
})

it('returns version if present', function () {
expect(
getDependencyVersion({
wantedDependency: 'chardet',
lockfileData: {
default: packages,
develop: {},
},
})
).to.deep.equal({ version: '3.0.4' })
})

it('returns (complete) ref if ref is tag', function () {
expect(
getDependencyVersion({
wantedDependency: 'django',
lockfileData: {
default: packages,
develop: {},
},
})
).to.deep.equal({ ref: '1.11.4001' })
})

it('returns truncated ref if ref is commit hash', function () {
expect(
getDependencyVersion({
wantedDependency: 'discordlists-py',
lockfileData: {
default: packages,
develop: {},
},
})
).to.deep.equal({ ref: '2df5a2b' })
})

it('throws if no version or ref', function () {
expect(() =>
getDependencyVersion({
wantedDependency: 'django-cms',
lockfileData: {
default: packages,
develop: {},
},
})
)
.to.throw(InvalidParameter)
.with.property('prettyMessage', 'No version or ref for django-cms')
})
})