Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor(semver): clean up parseRange, add missing tests #6362

Merged
merged 13 commits into from
Feb 12, 2025
49 changes: 48 additions & 1 deletion semver/compare_test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Copyright 2018-2025 the Deno authors. MIT license.
import { assertEquals } from "@std/assert";
import { assertEquals, assertThrows } from "@std/assert";
import { parse } from "./parse.ts";
import { compare } from "./compare.ts";

Expand Down Expand Up @@ -54,3 +54,50 @@ Deno.test({
}
},
});

Deno.test({
name: "compare() throws on NaN",
fn: () => {
assertThrows(
() =>
compare({ major: NaN, minor: 0, patch: 0 }, {
major: 1,
minor: 0,
patch: 0,
}),
Error,
"Cannot compare against non-numbers",
);

assertThrows(
() =>
compare({ major: 1, minor: 0, patch: 0 }, {
major: 1,
minor: NaN,
patch: 0,
}),
Error,
"Cannot compare against non-numbers",
);
},
});

Deno.test({
name: "compare() handles undefined in prerelease",
Copy link
Member

Choose a reason for hiding this comment

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

This test case looks comparing invalid semvers. I don't think this is an intentional behavior, but an undefined behavior. I'd suggest we should remove this test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is undefined behavior we probably should remove if (a === undefined && b === undefined) return 0; from compareIdentifier() as this is specifically handled there.

fn: () => {
assertEquals(
compare({
major: 1,
minor: 0,
patch: 0,
prerelease: [undefined as unknown as string],
}, {
major: 1,
minor: 0,
patch: 0,
prerelease: [undefined as unknown as string],
}),
0,
);
},
});
42 changes: 41 additions & 1 deletion semver/greater_than_range_test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
// Copyright 2018-2025 the Deno authors. MIT license.
// Copyright Isaac Z. Schlueter and npm contributors. All rights reserved. ISC license.

import { assert, assertFalse } from "@std/assert";
import { assert, assertEquals, assertFalse } from "@std/assert";
import {
format,
formatRange,
greaterThanRange,
parse,
parseRange,
} from "./mod.ts";
import type { Operator } from "./types.ts";

Deno.test("greaterThanRange() checks if the semver is greater than the range", async (t) => {
// from https://github.com/npm/node-semver/blob/692451bd6f75b38a71a99f39da405c94a5954a22/test/fixtures/version-gt-range.js
Expand Down Expand Up @@ -167,3 +168,42 @@ Deno.test("greaterThanRange() checks if the semver is greater than the range", a
});
}
});

Deno.test("greaterThanRange() handles equals operator", () => {
const version = {
major: 1,
minor: 0,
patch: 0,
prerelease: [],
build: [],
};
const range = [[{
operator: "=" as unknown as Operator,
major: 1,
minor: 0,
patch: 0,
prerelease: [],
build: [],
}]];
assertEquals(greaterThanRange(version, range), false);
});

Deno.test("greaterThanRange() handles not equals operator", () => {
const version = {
major: 1,
minor: 0,
patch: 0,
prerelease: [],
build: [],
};
const range = [[{
operator: "!=" as const,
major: 1,
minor: 0,
patch: 0,
prerelease: [],
build: [],
}]];
// FIXME(kt3k): This demonstrates a bug. This should be false
assertEquals(greaterThanRange(version, range), true);
timreichen marked this conversation as resolved.
Show resolved Hide resolved
});
66 changes: 52 additions & 14 deletions semver/is_range_test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
// Copyright 2018-2025 the Deno authors. MIT license.
import { assert } from "@std/assert";
import { assertEquals } from "@std/assert";
import { ALL, MIN } from "./_constants.ts";
import { formatRange } from "./format_range.ts";
import { isRange } from "./is_range.ts";
import type { Range } from "./types.ts";

Deno.test({
name: "isRange()",
fn: async (t) => {
const ranges: Range[] = [[
[ALL],
], [
name: "isRange() handles simple range",
fn: () => {
const range: Range = [
[{
operator: ">=",
major: 0,
Expand All @@ -22,12 +19,53 @@ Deno.test({
operator: "<",
...MIN,
}],
]];
for (const r of ranges) {
await t.step(`${formatRange(r)}`, () => {
const actual = isRange(r);
assert(actual);
});
}
];
const actual = isRange(range);
assertEquals(actual, true);
},
});

Deno.test({
name: "isRange() handles ALL constant",
fn: () => {
const range: Range = [[ALL]];
const actual = isRange(range);
assertEquals(actual, true);
},
});

Deno.test({
name: "isRange() handles null",
fn: () => {
const range: Range = [[null]] as unknown as Range;
const actual = isRange(range);
assertEquals(actual, false);
},
});

Deno.test({
name: "isRange() handles undefined",
fn: () => {
const range: Range = [[undefined]] as unknown as Range;
const actual = isRange(range);
assertEquals(actual, false);
},
});

Deno.test({
name: "isRange() handles array type",
fn: () => {
const range: Range = [[[1, 2, 3]]] as unknown as Range;
const actual = isRange(range);
assertEquals(actual, false);
},
});

Deno.test({
name: "isRange() handles not object type",
fn: () => {
const range: Range = [[[true]]] as unknown as Range;
const actual = isRange(range);
assertEquals(actual, false);
},
});
3 changes: 2 additions & 1 deletion semver/is_semver_test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Copyright 2018-2025 the Deno authors. MIT license.
import { assert } from "@std/assert";
import { MIN } from "./_constants.ts";
import { ANY, MIN } from "./_constants.ts";
import { isSemVer } from "./is_semver.ts";

Deno.test({
Expand Down Expand Up @@ -61,6 +61,7 @@ Deno.test({
[{ major: 0, minor: 0, patch: 0, build: [], prerelease: ["abc"] }],
[{ major: 0, minor: 0, patch: 0, build: [], prerelease: ["abc", 0] }],
[MIN],
[ANY],
];
for (const [v] of versions) {
await t.step(
Expand Down
22 changes: 21 additions & 1 deletion semver/less_than_range_test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2018-2025 the Deno authors. MIT license.
// Copyright Isaac Z. Schlueter and npm contributors. All rights reserved. ISC license.

import { assert, assertFalse } from "@std/assert";
import { assert, assertEquals, assertFalse } from "@std/assert";
import {
format,
formatRange,
Expand Down Expand Up @@ -169,3 +169,23 @@ Deno.test("lessThanRange() checks if the SemVer is less than the range", async (
});
}
});

Deno.test("lessThanRange() handles not equals operator", () => {
const version = {
major: 1,
minor: 0,
patch: 0,
prerelease: [],
build: [],
};
const range = [[{
operator: "!=" as const,
major: 1,
minor: 0,
patch: 0,
prerelease: [],
build: [],
}]];
// FIXME(kt3k): This demonstrates a bug. This should be false
assertEquals(lessThanRange(version, range), true);
Copy link
Member

Choose a reason for hiding this comment

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

This result feels wrong to me. I'd suggest skipping this test case for now

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 think we should not skip test cases if they yield unexpected results but fix them instead. What would the proper result be?

Copy link
Member

Choose a reason for hiding this comment

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

What would the proper result be?

The range != 1.0.0 should include all versions except 1.0.0. So I think the expected result here is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to create a PR to remove the != straight away as a bug fix before landing this PR? This probably solves lots of headaches with intermediate patches.

Copy link
Member

Choose a reason for hiding this comment

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

That will be a breaking change and we currently don't have a plan for it.

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 think this bug could be fixed by modifying the != case to return cmp === 0; instead of return false; in greaterThanComparator() and lessThanComparator(). Then it will return the expected false.

});
16 changes: 7 additions & 9 deletions semver/parse_range.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,27 +141,25 @@ function handleRightHyphenRangeGroups(
major: +rightGroups.major,
minor: +rightGroups.minor,
patch: +rightGroups.patch,
prerelease: rightGroups.prerelease
? parsePrerelease(rightGroups.prerelease)
: [],
prerelease: [],
build: [],
};
}
function parseHyphenRange(range: string): Comparator[] | undefined {
function parseHyphenRange(range: string): Comparator[] | null {
const leftMatch = range.match(new RegExp(`^${XRANGE}`));
const leftGroup = leftMatch?.groups;
if (!leftGroup) return;
if (!leftGroup) return null;
const leftLength = leftMatch[0].length;

const hyphenMatch = range.slice(leftLength).match(/^\s+-\s+/);
if (!hyphenMatch) return;
if (!hyphenMatch) return null;
const hyphenLength = hyphenMatch[0].length;

const rightMatch = range.slice(leftLength + hyphenLength).match(
new RegExp(`^${XRANGE}\\s*$`),
);
const rightGroups = rightMatch?.groups;
if (!rightGroups) return;
if (!rightGroups) return null;

const from = handleLeftHyphenRangeGroups(leftGroup as RangeRegExpGroups);
const to = handleRightHyphenRangeGroups(rightGroups as RangeRegExpGroups);
Expand Down Expand Up @@ -255,7 +253,7 @@ function handleLessThanOperator(groups: RangeRegExpGroups): Comparator[] {
if (majorIsWildcard) return [{ operator: "<", major: 0, minor: 0, patch: 0 }];
if (minorIsWildcard) {
if (patchIsWildcard) return [{ operator: "<", major, minor: 0, patch: 0 }];
return [{ operator: "<", major, minor, patch: 0 }];
return [{ operator: "<", major, minor: 0, patch: 0 }];
}
if (patchIsWildcard) return [{ operator: "<", major, minor, patch: 0 }];
const prerelease = parsePrerelease(groups.prerelease ?? "");
Expand Down Expand Up @@ -318,7 +316,7 @@ function handleGreaterOrEqualOperator(groups: RangeRegExpGroups): Comparator[] {
if (majorIsWildcard) return [ALL];
if (minorIsWildcard) {
if (patchIsWildcard) return [{ operator: ">=", major, minor: 0, patch: 0 }];
return [{ operator: ">=", major, minor, patch: 0 }];
return [{ operator: ">=", major, minor: 0, patch: 0 }];
}
if (patchIsWildcard) return [{ operator: ">=", major, minor, patch: 0 }];
const prerelease = parsePrerelease(groups.prerelease ?? "");
Expand Down
21 changes: 21 additions & 0 deletions semver/parse_range_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -666,3 +666,24 @@ Deno.test("parseRange() throws on invalid range", () => {
'Cannot parse version range: range "blerg" is invalid',
);
});

Deno.test("parseRange() handles wildcards", () => {
assertEquals(parseRange("<1.*"), [
[{ operator: "<", major: 1, minor: 0, patch: 0 }],
]);
assertEquals(parseRange("<1.*.0"), [
[{ operator: "<", major: 1, minor: 0, patch: 0 }],
]);
assertEquals(parseRange("<1.*.*"), [
[{ operator: "<", major: 1, minor: 0, patch: 0 }],
]);
assertEquals(parseRange(">=1.*.0"), [
[{ operator: ">=", major: 1, minor: 0, patch: 0 }],
]);
assertEquals(parseRange(">=1.*.*"), [
[{ operator: ">=", major: 1, minor: 0, patch: 0 }],
]);
assertEquals(parseRange(">=1.0.*"), [
[{ operator: ">=", major: 1, minor: 0, patch: 0 }],
]);
});
34 changes: 34 additions & 0 deletions semver/try_parse_range_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2018-2025 the Deno authors. MIT license.

import { assertEquals } from "@std/assert";
import { tryParseRange } from "./try_parse_range.ts";
import type { Range } from "./types.ts";

Deno.test("tryParseRange()", () => {
const actual = tryParseRange(">=1.2.3 <1.2.4");
const expected: Range = [
[
{
operator: ">=",
major: 1,
minor: 2,
patch: 3,
prerelease: [],
build: [],
},
{
operator: "<",
major: 1,
minor: 2,
patch: 4,
prerelease: [],
build: [],
},
],
];
assertEquals(actual, expected);
});

Deno.test("tryParseRange() handles invalid range", () => {
assertEquals(tryParseRange("blerg"), undefined);
});
1 change: 1 addition & 0 deletions semver/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export type ReleaseType =
export type Operator =
| undefined
| "="
// Note: `!=` operator type does not exist in npm:semver
| "!="
| ">"
| ">="
Expand Down
Loading