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.

Sign up for GitHub

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

Jump to bottom

Fix package id case-insensitivity in different places #1

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Fix package id case-insensitivity in different places #1

wants to merge 6 commits into from

Conversation

derigel23
Copy link
Contributor

No description provided.

@emgarten
Copy link
Member

@derigel23 please add some unit tests for this

@yishaigalatzer
Copy link
Contributor

@derigel23 preferrably, please elaborate on what is the failing scenario here? Provide unit tests that fail before the fix and pass after.

@yishaigalatzer
Copy link
Contributor

@derigel23 we can't keep this open without progress. Please let us know if you intend to put more work here.

@derigel23
Copy link
Contributor Author

@yishaigalatzer I'll make a test in day or two

@derigel23
Copy link
Contributor Author

I've added indirect test for PackageEqualityComparer.Id
Without my fix tests will be failed but they shouldn't.
Also, discovered some other bugs: ListCommand.GetPackages is case-sensitive, but should be case-insensitive since package id is case-insensitive.
The same error in GetPackageCommand.

@derigel23 derigel23 changed the title Correct PackageEqualityComparer for using in hashtables Fix package id case-insensitivity in different places Sep 3, 2015
yishaigalatzer
yishaigalatzer reviewed Sep 3, 2015
View reviewed changes
test/CommandLine.Test/ListCommandTests.cs
@@ -180,7 +180,7 @@ public void GetPackageReturnsAllVersionsIfAllVersionsFlagIsSet()

// Assert
Assert.Equal(5, packages.Count());
AssertPackage(new { Id = "jQuery", Ver = "1.44" }, packages.ElementAt(0));
AssertPackage(new { Id = "JQuery", Ver = "1.44" }, packages.ElementAt(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a separate test, each test should represent one feature/behavior tested to the degree possible

@yishaigalatzer
Copy link
Contributor

@emgarten can you please review and merge for 3.3?

@MeniZalzman MeniZalzman assigned zhili1208 and unassigned zhili1208 Oct 2, 2015
@yishaigalatzer
Copy link
Contributor

@emgarten ping

@derigel23
Copy link
Contributor Author

@yishaigalatzer @emgarten ping

@emgarten
Copy link
Member

@derigel23 I'll take another look at merging this for 2.12

@ghost ghost removed the cla-not-required label Dec 6, 2017
@ghost ghost deleted a comment from dnfclas Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@derigel23 @emgarten @yishaigalatzer @zhili1208 @dnfclas