test(semver): add missing tests #6362
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6362 +/- ##
==========================================
+ Coverage 96.23% 96.29% +0.06%
==========================================
Files 556 556
Lines 42065 42063 -2
Branches 6371 6371
==========================================
+ Hits 40481 40506 +25
+ Misses 1544 1518 -26
+ Partials 40 39 -1 ☔ View full report in Codecov by Sentry. |
| prerelease: [], | ||
| build: [], | ||
| }]]; | ||
| assertEquals(greaterThanRange(version, range), true); |
There was a problem hiding this comment.
I wonder if this result is correct
I also just realized != is not a valid operator in npm:semver (semver.gtr("1.0.0", "!=1.0.0") throws with invalid operator error, and docs doesn't mention != https://github.com/npm/node-semver?tab=readme-ov-file#ranges
I'd suggest we should skip this particular test case for now.
There was a problem hiding this comment.
Instead of skipping, we probably should add the test and remove it when handling the problem/bug.
There was a problem hiding this comment.
Then can you add a TODO comment above the test title, which says that this is a bug, not an intentional behavior?
| prerelease: [], | ||
| build: [], | ||
| }]]; | ||
| assertEquals(lessThanRange(version, range), true); |
There was a problem hiding this comment.
This result feels wrong to me. I'd suggest skipping this test case for now
There was a problem hiding this comment.
I think we should not skip test cases if they yield unexpected results but fix them instead. What would the proper result be?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| assertEquals(parseRange("<1.*"), [ | ||
| [{ operator: "<", major: 1, minor: 0, patch: 0 }], | ||
| ]); | ||
| assertEquals(parseRange("<1.*.0"), [ | ||
| [{ operator: "<", major: 1, minor: NaN, patch: 0 }], | ||
| ]); |
There was a problem hiding this comment.
The difference of these 2 results seem strange to me. Is this intentionally in this way?
There was a problem hiding this comment.
This seems indeed wrong. Imo <1.*.0 should be an invalid range and throw.
There was a problem hiding this comment.
I took a closer look at this, satisfies() tests allow this:
https://github.com/denoland/std/blob/3b75ee7c1925388e01dd69540d51d9387e26dcb0/semver/satisfies_test.ts#L75C5-L75C25
| }); | ||
|
|
||
| Deno.test({ | ||
| name: "compare() handles undefined in prerelease", |
There was a problem hiding this comment.
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.
No description provided.