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

Update SPDX Expression Parsing #719

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 10 additions & 17 deletions __tests__/config.test.ts
@@ -1,13 +1,9 @@
import {expect, test, beforeEach} from '@jest/globals'
import {readConfig} from '../src/config'
import {getRefs} from '../src/git-refs'
import * as Utils from '../src/utils'
import * as spdx from '../src/spdx'
import {setInput, clearInputs} from './test-helpers'

beforeAll(() => {
jest.spyOn(Utils, 'isSPDXValid').mockReturnValue(true)
})

beforeEach(() => {
clearInputs()
})
Expand All @@ -19,11 +15,11 @@ test('it defaults to low severity', async () => {

test('it reads custom configs', async () => {
setInput('fail-on-severity', 'critical')
setInput('allow-licenses', ' BSD, GPL 2')
setInput('allow-licenses', 'ISC, GPL-2.0')

const config = await readConfig()
expect(config.fail_on_severity).toEqual('critical')
expect(config.allow_licenses).toEqual(['BSD', 'GPL 2'])
expect(config.allow_licenses).toEqual(['ISC', 'GPL-2.0'])
})

test('it defaults to false for warn-only', async () => {
Expand All @@ -40,14 +36,15 @@ test('it defaults to empty allow/deny lists ', async () => {

test('it raises an error if both an allow and denylist are specified', async () => {
setInput('allow-licenses', 'MIT')
setInput('deny-licenses', 'BSD')
setInput('deny-licenses', 'BSD-3-Clause')

await expect(readConfig()).rejects.toThrow(
'You cannot specify both allow-licenses and deny-licenses'
)
})

test('it raises an error if an empty allow list is specified', async () => {
setInput('config-file', './__tests__/fixtures/config-empty-allow-sample.yml')
setInput('config-file', '../__tests__/fixtures/config-empty-allow-sample.yml')

await expect(readConfig()).rejects.toThrow(
'You should provide at least one license in allow-licenses'
Expand Down Expand Up @@ -158,21 +155,17 @@ test('it is not possible to disable both checks', async () => {
})

describe('licenses that are not valid SPDX licenses', () => {
beforeAll(() => {
jest.spyOn(Utils, 'isSPDXValid').mockReturnValue(false)
})

test('it raises an error for invalid licenses in allow-licenses', async () => {
setInput('allow-licenses', ' BSD, GPL 2')
setInput('allow-licenses', 'BSD')
await expect(readConfig()).rejects.toThrow(
'Invalid license(s) in allow-licenses: BSD,GPL 2'
'Invalid license(s) in allow-licenses: BSD'
)
})

test('it raises an error for invalid licenses in deny-licenses', async () => {
setInput('deny-licenses', ' BSD, GPL 2')
setInput('deny-licenses', 'BSD')
await expect(readConfig()).rejects.toThrow(
'Invalid license(s) in deny-licenses: BSD,GPL 2'
'Invalid license(s) in deny-licenses: BSD'
)
})
})
Expand Down
12 changes: 8 additions & 4 deletions __tests__/deny.test.ts
@@ -1,5 +1,6 @@
import {expect, jest, test} from '@jest/globals'
import {Change, Changes} from '../src/schemas'
import * as spdx from '../src/spdx'

let getDeniedChanges: Function

Expand Down Expand Up @@ -121,10 +122,13 @@ jest.mock('octokit', () => {

beforeEach(async () => {
jest.resetModules()
jest.doMock('spdx-satisfies', () => {
// mock spdx-satisfies return value
// true for BSD, false for all others
return jest.fn((license: string, _: string): boolean => license === 'BSD')
jest.mock('../src/spdx', () => {
return {
...spdx,
satisfies: jest.fn(
(license: string, _: string): boolean => license === 'BSD'
)
}
})
// eslint-disable-next-line @typescript-eslint/no-require-imports
;({getDeniedChanges} = require('../src/deny'))
Expand Down
26 changes: 11 additions & 15 deletions __tests__/external-config.test.ts
@@ -1,6 +1,7 @@
import * as spdx from '../src/spdx'
import * as path from 'path'
import {expect, test, beforeEach} from '@jest/globals'
import {readConfig} from '../src/config'
import * as Utils from '../src/utils'
import {setInput, clearInputs} from './test-helpers'

const externalConfig = `fail_on_severity: 'high'
Expand All @@ -25,20 +26,15 @@ jest.mock('octokit', () => {
}
})

beforeAll(() => {
jest.spyOn(Utils, 'isSPDXValid').mockReturnValue(true)
})

beforeEach(() => {
clearInputs()
})

test('it reads an external config file', async () => {
setInput('config-file', './__tests__/fixtures/config-allow-sample.yml')

setInput('config-file', '../__tests__/fixtures/config-allow-sample.yml')
const config = await readConfig()
expect(config.fail_on_severity).toEqual('critical')
expect(config.allow_licenses).toEqual(['BSD', 'GPL 2'])
expect(config.allow_licenses).toEqual(['BSD-3-Clause', 'GPL-2.0'])
})

test('raises an error when the config file was not found', async () => {
Expand All @@ -47,7 +43,7 @@ test('raises an error when the config file was not found', async () => {
})

test('it parses options from both sources', async () => {
setInput('config-file', './__tests__/fixtures/config-allow-sample.yml')
setInput('config-file', '../__tests__/fixtures/config-allow-sample.yml')

let config = await readConfig()
expect(config.fail_on_severity).toEqual('critical')
Expand All @@ -59,38 +55,38 @@ test('it parses options from both sources', async () => {

test('in case of conflicts, the inline config is the source of truth', async () => {
setInput('fail-on-severity', 'low')
setInput('config-file', './__tests__/fixtures/config-allow-sample.yml') // this will set fail-on-severity to 'critical'
setInput('config-file', '../__tests__/fixtures/config-allow-sample.yml') // this will set fail-on-severity to 'critical'

const config = await readConfig()
expect(config.fail_on_severity).toEqual('low')
})

test('it uses the default values when loading external files', async () => {
setInput('config-file', './__tests__/fixtures/no-licenses-config.yml')
setInput('config-file', '../__tests__/fixtures/no-licenses-config.yml')
let config = await readConfig()
expect(config.allow_licenses).toEqual(undefined)
expect(config.deny_licenses).toEqual(undefined)

setInput('config-file', './__tests__/fixtures/license-config-sample.yml')
setInput('config-file', '../__tests__/fixtures/license-config-sample.yml')
config = await readConfig()
expect(config.fail_on_severity).toEqual('low')
})

test('it accepts an external configuration filename', async () => {
setInput('config-file', './__tests__/fixtures/no-licenses-config.yml')
setInput('config-file', '../__tests__/fixtures/no-licenses-config.yml')
const config = await readConfig()
expect(config.fail_on_severity).toEqual('critical')
})

test('it raises an error when given an unknown severity in an external config file', async () => {
setInput('config-file', './__tests__/fixtures/invalid-severity-config.yml')
setInput('config-file', '../__tests__/fixtures/invalid-severity-config.yml')
await expect(readConfig()).rejects.toThrow()
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 do these paths require the ../__tests__/ prefix or would a fixtures/ prefix work too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure why (maybe jest?), but the path needs to be relative to src/, hence the change from ./ to ../:

[FAIL] it accepts an external configuration filename

    Unable to fetch or parse config file: ENOENT: no such file or directory, open '/Users/febuiles/w/dr-action/src/fixtures/no-licenses-config.yml'

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @hmaurer @bteng22 thoughts on this? if its nothing, feel free to ignore 👍

})

test('it supports comma-separated lists', async () => {
setInput(
'config-file',
'./__tests__/fixtures/inline-license-config-sample.yml'
'../__tests__/fixtures/inline-license-config-sample.yml'
)
const config = await readConfig()

Expand Down
4 changes: 2 additions & 2 deletions __tests__/fixtures/config-allow-sample.yml
@@ -1,4 +1,4 @@
fail_on_severity: critical
allow_licenses:
- "BSD"
- "GPL 2"
- "BSD-3-Clause"
- "GPL-2.0"
25 changes: 16 additions & 9 deletions __tests__/licenses.test.ts
@@ -1,5 +1,6 @@
import {expect, jest, test} from '@jest/globals'
import {Change, Changes} from '../src/schemas'
import * as spdx from '../src/spdx'

let getInvalidLicenseChanges: Function

Expand Down Expand Up @@ -100,10 +101,13 @@ jest.mock('octokit', () => {

beforeEach(async () => {
jest.resetModules()
jest.doMock('spdx-satisfies', () => {
// mock spdx-satisfies return value
// true for BSD, false for all others
return jest.fn((license: string, _: string): boolean => license === 'BSD')
jest.mock('../src/spdx', () => {
return {
...spdx,
satisfies: jest.fn(
(license: string, _: string): boolean => license === 'BSD'
)
}
})
// eslint-disable-next-line @typescript-eslint/no-require-imports
;({getInvalidLicenseChanges} = require('../src/licenses'))
Expand Down Expand Up @@ -162,11 +166,14 @@ test('it adds license outside the allow list to forbidden changes if it is in bo
})

test('it adds all licenses to unresolved if it is unable to determine the validity', async () => {
jest.resetModules() // reset module set in before
jest.doMock('spdx-satisfies', () => {
return jest.fn((_first: string, _second: string) => {
throw new Error('Some Error')
})
jest.resetModules()
jest.mock('../src/spdx', () => {
return {
...spdx,
satisfies: jest.fn((_l: string, _e: string): boolean => {
throw new Error('Some Error')
})
}
})
// eslint-disable-next-line @typescript-eslint/no-require-imports
;({getInvalidLicenseChanges} = require('../src/licenses'))
Expand Down
53 changes: 53 additions & 0 deletions __tests__/spdx.test.ts
@@ -0,0 +1,53 @@
import {expect, test, describe} from '@jest/globals'
import * as spdx from '../src/spdx'

describe('isValid', () => {
test('returns true on valid expressions', async () => {
const license = 'MIT'
expect(spdx.isValid(license)).toBe(true)
})

test('returns false on invalid expressions', async () => {
const license = 'nope'
expect(spdx.isValid(license)).toBe(false)
})
})

describe('satisfies', () => {
test('returns true if a license satisfies a constraint', async () => {
const license = 'MIT'
const expr = 'MIT OR GPL-2.0'
expect(spdx.satisfies(license, expr)).toBe(true)
})

test('works on AND expressions', () => {
const license = 'GPL-2.0 AND GPL-3.0'
const expr = 'MIT OR (GPL-2.0 AND GPL-3.0)'
expect(spdx.satisfies(license, expr)).toBe(true)
})

test('-only expressions are properly parsed', () => {
const license = 'GPL-3.0'
const expr = 'GPL-3.0-only'
expect(spdx.satisfies(license, expr)).toBe(true)
})

test('-or-later expressions are properly parsed', () => {
const license = 'GPL-3.0'
const expr = 'GPL-2.0-or-later'
expect(spdx.satisfies(license, expr)).toBe(true)
})

test('GPL expressions are supported', () => {
const license = 'GPL-2.0'
const expr = 'GPL-3.0-or-later'
expect(spdx.satisfies(license, expr)).toBe(false)
})

test('returns false if no matches are found', async () => {
const license = 'ISC'
const expr = 'MIT OR GPL-2.0'

expect(spdx.satisfies(license, expr)).toBe(false)
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

😍