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

test(semver): add test for parseRange() #4345

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Conversation

javihernant
Copy link
Contributor

No description provided.

@javihernant
Copy link
Contributor Author

javihernant commented Feb 16, 2024

This is just a draft. I am not sure if test should be implemented like this. Can anyone confirm? Also, I am new at TS and probably there's a more idiomatic way to write it. So, please, I am happy to hear about anything wrong or that can be improved. Ty!!

return true;
}

function equalRanges(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this util. We can use assertEquals to compare nested objects.

@javihernant
Copy link
Contributor Author

javihernant commented Feb 18, 2024

This new commit contains one way to test parseRange(). The only problem I see here is, if parseRange() implementation is slightly changed so that the object returned is a bit different from that in the expected array (in parse_args_test.ts), then the tests are going to fail. For example, when I originally made the parse_args_test.ts file, all tests worked fine. However, today I have rechecked and one of them didn't work because one of the objects returned had changed. It changed from having the property "operator" as an empty string to an undefined value. How do you think we can tackle this issue? What changes can I make to this PR? Please, don't hesitate to ask me to change anything. I'm happy to make any changes in order to have this working! :)

@kt3k
Copy link
Member

kt3k commented Feb 19, 2024

The only problem I see here is, if parseRange() implementation is slightly changed so that the object returned is a bit different from that in the expected array (in parse_args_test.ts), then the tests are going to fail. For example, when I originally made the parse_args_test.ts file, all tests worked fine. However, today I have rechecked and one of them didn't work because one of the objects returned had changed. It changed from having the property "operator" as an empty string to an undefined value. How do you think we can tackle this issue?

I think that's just fine. Tests are for catching such changes. We'll update the test expectation in that cases if the change looks reasonable.

}
});

Deno.test("parseHyphenRanges", () => {
Copy link
Member

Choose a reason for hiding this comment

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

We have some rule for naming test cases: https://github.com/denoland/deno_std/blob/main/.github/CONTRIBUTING.md#tests

This case probably should be something like:

Suggested change
Deno.test("parseHyphenRanges", () => {
Deno.test("parseRange() parses ranges with hyphens", () => {

Can you also update the other test cases alike?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@javihernant javihernant marked this pull request as ready for review February 19, 2024 08:59
@kt3k kt3k changed the title chore(semver): add test for parse_range (4309) test(semver): add test for parse_range Feb 21, 2024
Comment on lines 55 to 60
["^1.2.3+build", [
[
{ operator: ">=", major: 1, minor: 2, patch: 3, prerelease: [] },
{ operator: "<", major: 2, minor: 0, patch: 0 },
],
]],
Copy link
Member

Choose a reason for hiding this comment

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

This one seems the same as the above one?

},
],
]],
["^1.2.3", [
Copy link
Member

Choose a reason for hiding this comment

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

This case seems appearing twice

["=1.2.3", [
[
{
operator: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why this isn't "="? 🤔

Copy link
Contributor Author

@javihernant javihernant Feb 22, 2024

Choose a reason for hiding this comment

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

I think that previously was operator:"". I'm going to check this

},
],
]],
[">=1.0.0", [
Copy link
Member

Choose a reason for hiding this comment

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

Looks the same as the above one

},
],
]],
[">1.0.0", [
Copy link
Member

Choose a reason for hiding this comment

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

Looks a duplicate

@kt3k
Copy link
Member

kt3k commented Feb 21, 2024

@javihernant Thanks for updating! Looks close to be merged, but I found several cases were repeated with completely the same input and expectations. Can you clean up them?

},
],
]],
["<=2.0.0", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

},
],
]],
["<=2.0.0", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

},
],
]],
["<2.0.0", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

},
],
]],
[">=0.2.3 || <0.0.1", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 3, minor: 0, patch: 0 },
],
]],
["1.2.x || 2.x", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 3, minor: 0, patch: 0 },
],
]],
["2.*.*", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 3, minor: 0, patch: 0 },
],
]],
["1.2.* || 2.*", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 0, minor: 1, patch: 0 },
],
]],
["~0.0.1", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 2, minor: 5, patch: 0 },
],
]],
["~2.4", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 3, minor: 3, patch: 0 },
],
]],
["~>3.2.1", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 2, minor: 0, patch: 0 },
],
]],
["~>1", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 0, minor: 0, patch: 2 },
],
]],
["^0.0.1", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 0, minor: 0, patch: 2 },
],
]],
["^1.2.3", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 2, minor: 0, patch: 0 },
],
]],
["^1.2.3", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

{ operator: "<", major: 2, minor: 0, patch: 0 },
],
]],
["^1.2", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

},
],
]],
["^1.2.3", [
Copy link
Member

Choose a reason for hiding this comment

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

seems a duplicate

@kt3k
Copy link
Member

kt3k commented Feb 21, 2024

I think I found too many duplicated entries.. I might be missing something..

@javihernant
Copy link
Contributor Author

Updated the tests to remove all duplicates. I'm sorry I left so many!!!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this!

@kt3k kt3k merged commit 3e42606 into denoland:main Feb 22, 2024
12 checks passed
@iuioiua iuioiua changed the title test(semver): add test for parse_range test(semver): add test for parseRange() Feb 23, 2024
eryue0220 added a commit to eryue0220/deno_std that referenced this pull request Feb 28, 2024
…0/deno_std into fix/expect-custom-equality-case

* 'fix/expect-custom-equality-case' of github.com:eryue0220/deno_std: (63 commits)
  docs: link to `assertThrows()` and `assertRejects()` (denoland#4395)
  chore(log): sync `level` and `levelName` in BaseHandler (denoland#4393)
  docs: ignore bad snippet examples (denoland#4388)
  chore(media_types): format test names (denoland#4380)
  docs: clarify underscore guidance in README (denoland#4385)
  feat(collections): add `pick` and `omit` (denoland#4218)
  chore(msgpack): format test names (denoland#4381)
  refactor(encoding): prepare for `noUncheckedIndexedAccess` (denoland#4275)
  refactor(streams): prepare for `noUncheckedIndexedAccess` (denoland#4377)
  chore: fix .editorconfig syntax (denoland#4376)
  chore(semver): remove legacy `Range.ranges` object definition (denoland#4374)
  chore(semver): move breaking versions (denoland#4372)
  refactor(semver): rename `comparatorFormat()` to `formatComparator()` (denoland#4373)
  test(semver): add test for parse_range (denoland#4345)
  chore: use 'release' event for triggering jsr publish (denoland#4370)
  chore(http): fix spawned tests after migration script (denoland#4368)
  chore(crypto): move test scripts to own files (denoland#4367)
  0.217.0 (denoland#4369)
  build: update _ to - in workspace converter script (denoland#4357)
  chore(media_types): move `extensions` utility (denoland#4358)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants