Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

filter test typos and another test case #7891

Closed
rolfyone opened this issue Jun 18, 2014 · 6 comments
Closed

filter test typos and another test case #7891

rolfyone opened this issue Jun 18, 2014 · 6 comments
Milestone

Comments

@rolfyone
Copy link
Contributor

There was a test submission earlier regarding filtering on empty text.

While investigating, there were a couple of typos I found, and I added a test case showing the described issue, and how to solve the problem of filtering on an empty string.

I'm not sure how to submit it, whether to push my tested change etc, so I figured I'd raise it as an issue, and if someone wants to push the commit themselves I dont mind... I tried to find things in 'getting started', but couldnt really find any newbie dev guidelines that covered this kind of thing.... no doubt I missed a huge section of doco somewhere...

anyway a diff is

diff --git a/test/ng/filter/filterSpec.js b/test/ng/filter/filterSpec.js
index fecfcf7..8398949 100644
--- a/test/ng/filter/filterSpec.js
+++ b/test/ng/filter/filterSpec.js
@@ -49,7 +49,7 @@ describe('Filter: filter', function() {
     expect(filter(items, function(i) {return i.done;}).length).toBe(1);
   });

-  it('should take object as perdicate', function() {
+  it('should take object as predicate', function() {
     var items = [{first: 'misko', last: 'hevery'},
                  {first: 'adam', last: 'abrons'}];

@@ -61,7 +61,7 @@ describe('Filter: filter', function() {
   });


-  it('should support predicat object with dots in the name', function() {
+  it('should support predicate object with dots in the name', function() {
     var items = [{'first.name': 'misko', 'last.name': 'hevery'},
                  {'first.name': 'adam', 'last.name': 'abrons'}];

@@ -81,6 +81,16 @@ describe('Filter: filter', function() {
     ]);
   });

+  it('should be able to use exact match to match empty strings', function() {
+    var items = [{person: {name: 'John'}},
+                 {person: {name: 'Rita'}},
+                 {person: {name: 'Billy'}},
+                 {person: {name: ''}},
+                 {person: {name: 'Joan'}}];
+    expect(filter(items, {person: {name: ''}}).length).toBe(5);
+    expect(filter(items, {person: {name: ''}}, true).length).toBe(1);
+  });
+

   it('should match any properties for given "$" property', function() {
     var items = [{first: 'tom', last: 'hevery'},
@Narretz
Copy link
Contributor

Narretz commented Jun 18, 2014

Hi, thanks for the patch. If you want to give it a go, in https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#submitting-a-pull-request there's a pretty good overview of how to submit a pull request.

@rodyhaddad rodyhaddad added this to the Purgatory milestone Jun 18, 2014
@rolfyone
Copy link
Contributor Author

ok cool - i'll give it a go after work :)

@rolfyone
Copy link
Contributor Author

just so i remember, the initial issue was #7890

@rolfyone
Copy link
Contributor Author

still not able to do a git push :|

@caitp
Copy link
Contributor

caitp commented Jul 16, 2014

Hey, so part of the first commit has been merged, but the issue with the test added is that it's really testing something which is already being tested (the behaviour of the objectEquality flag).

So, we've opted to just remove the extra test, because the test serves as a sort of inaccurate way of documenting what that flag actually does. It's perfectly acceptable to use it in this way, but it's quite possible for it to actually not do what you really want under certain circumstances, so I prefer to keep that out.

However, google will serve this issue to people who find it, so if they're having trouble with that particular issue and need a quick fix for it, this should definitely help them find it.

Thanks @rolfyone !

@caitp caitp closed this as completed Jul 16, 2014
@rolfyone
Copy link
Contributor Author

no problems - thanks for your help :)

On Thu, Jul 17, 2014 at 7:08 AM, Caitlin Potter notifications@github.com
wrote:

Hey, so part of the first commit has been merged, but the issue with the
test added is that it's really testing something which is already being
tested (the behaviour of the objectEquality flag).

So, we've opted to just remove the extra test, because the test serves as
a sort of inaccurate way of documenting what that flag actually does. It's
perfectly acceptable to use it in this way, but it's quite possible for it
to actually not do what you really want under certain circumstances, so I
prefer to keep that out.

However, google will serve this issue to people who find it, so if they're
having trouble with that particular issue and need a quick fix for it, this
should definitely help them find it.

Thanks @rolfyone https://github.com/rolfyone !


Reply to this email directly or view it on GitHub
#7891 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants