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

Change scope to the property when using should.all.have.property #7

Open
Tyderion opened this issue Jan 29, 2015 · 26 comments
Open

Change scope to the property when using should.all.have.property #7

Tyderion opened this issue Jan 29, 2015 · 26 comments
Labels

Comments

@Tyderion
Copy link

Hi there
Thanks for creating this plugin it is quite useful.

One Problem I sometimes run into is the following.

In normal chai you can write something like this:

var a = {a: "a"};
a.should.have.property("a").not.equal("c"); // Test passes

var c = {a: "c"};
c.should.have.property("a").not.equal("c"); // Test fails

Is it possible to implement the same for all-validations?

Something like:

var a = {a: "a"};
var c = {a: "c"};
[a,c].should.all.have.property("a").not.equal("c"); // Test currently passes but should fail

That would be very handy.

@RubenVerborgh RubenVerborgh changed the title Change scope to the property when using should.all.have.property Change scope to the property when using should.all.have.property Jan 29, 2015
@RubenVerborgh RubenVerborgh self-assigned this Jan 29, 2015
@RubenVerborgh
Copy link
Collaborator

Thanks for reporting. Will look into this at some point.

RubenVerborgh added a commit that referenced this issue Jan 31, 2015
Work in progress for #7.
@RubenVerborgh
Copy link
Collaborator

I added test cases for the issue you mentioned, but a fix turns out to be hard / impossible in general. The way Chai is built, I cannot tell when to switch on or off the all or something flag. At the point the assertion happens, we simply don't know what will follow in the chain.

I'm afraid I'll thus have to close this issue unresolved. Should you know a solution that satisfies the added test cases, don't hesitate to let me know or add a pull request.

@keithamus
Copy link
Member

@RubenVerborgh or @Tyderion: just curious - have you filed an issue within chai about this? I'm not exactly sure how we could solve it - but I'd certainly like to at the very least discuss it in chai core.

@RubenVerborgh
Copy link
Collaborator

I haven't; I'm afraid it's not trivially solvable with the given chai API.

@keithamus
Copy link
Member

I think I might have an idea about fixing this though:

Chai's .property changes the flag(this, 'object') to be the property you're asserting on - meaning if you do expect({ a: 1 }).to.have.property('a') then the chained value is 1 - which means you can do cool stuff like expect({a: 1}).property('a').to.equal(1). I think this is the issue that is gumming up chai-things (please correct me if I'm wrong though). Based on this assumption I think we can solve this particular problem.

If chai-things were to specifically hook off of .property and change the object flag to be an /array/ of the gathered properties, it would change the value of the chained assertion - which could then fall through to the standard plugin behaviour.

In other words:

var arr = [ {a: 1}, {a: 1} ]
expect(arr).to.all.have.property('a'); // this works fine

var values = arr.filter(item => item.a)
expect(values).to.all.equal(1); // This works fine

// As such there is no reason why this shouldnt work:
var assertion = expect(arr).to.all.have.property('a');
flag(assertion, 'object', values);
assertion.to.equal(1);

Thoughts @RubenVerborgh? Do you think it'd work? Or do you think I'm way off base here?

@RubenVerborgh
Copy link
Collaborator

I must admit, it has been years since I've looked into this. I'm not even fully sure how I implemented it back then. Your idea seems to make sense indeed… although I'm afraid there are some more complexities hidden somewhere.

Unfortunately, I don't foresee having time to look into this in the near future, with other projects occupying most of my time. If somebody would volunteer, I'd be happy to offer support.

@keithamus
Copy link
Member

I'm happy to take a crack at a PR, I just wanted to gauge your opinion on it. So if you're happy I'll get working on it 😄

@RubenVerborgh
Copy link
Collaborator

That would be great, thanks! There are simple tests but they are currently skipped.

Do ensure that it integrates with the existing code though; if I recall correctly, we there hold a nested assertion object in order to be able to step back in the chain.
What would need to happen is that, at the moment the first assertion is tested on all elements of the array, the property it set should be remembered for possible chaining later.

I still think it will be quite hard to tackle cases like

[{ a: 1 }, { a: 2 }].should.include.something.with.property('a').equal(2);

because the first assertion would work on the first element, but the second wouldn't.

@keithamus
Copy link
Member

Just an update - I've done some work on this, and progress is looking good. I'll put a PR up shortly. My main problem so far is that the something and all flags do not behave like other chai flags - they do not carry onto the next assertions, making the current assertions quite weird:

// this does not work:
[{ a: 1 }, { a: 2 }].should.include.something.with.property('a').equal(2);

// but this does!
[{ a: 1 }, { a: 2 }].should.include.something.with.property('a').something.equal(2);

I'll get that resolved soon (I hope) and I'll get a PR ready for the next session I do on this.

@RubenVerborgh
Copy link
Collaborator

@keithamus You mentioned you had a partial pull request for this. I believe that would really change how this library would work.

Just a question, but would you be interested in taking ownership of Chai-Things? I am focusing on other JavaScript projects at the moment and can't give this library the attention it requires.

@keithamus
Copy link
Member

@RubenVerborgh I'd be happy to take over the project; I could pull it into the chaijs org, and make you a maintainer. The code would probably change quite significantly to come inline with Chai's code style though, to keep things consistent. Thoughts?

@RubenVerborgh
Copy link
Collaborator

Thanks! Can I transfer it to the chaijs organisation then?

I don't mind the code being changed. It's MIT, so go ahead!

@keithamus
Copy link
Member

Sweet, yeah simply transfer it to the chaijs org and I'll pick it up when I can

@RubenVerborgh
Copy link
Collaborator

I didn't have permission to transfer because I'm not an admin of chaijs, so I'm transferring it to your personal account instead. You should then be able to complete the transfer.

@RubenVerborgh
Copy link
Collaborator

Thanks, I see the transfer has been completed!

@keithamus
Copy link
Member

👍

@TueCN
Copy link

TueCN commented Feb 12, 2016

Hi @keithamus

Is there any updates to this issue?

I would really like to be able to use syntax like this for checking nested arrays:
someArray.should.all.have.property('someOtherArray').that.not.all.have.property('someProp')

Is there a recommended workaround?
I am not sure how to interpret your previous comment and make it work with allinstead of something

// this does not work:
[{ a: 1 }, { a: 2 }].should.include.something.with.property('a').equal(2);

// but this does!
[{ a: 1 }, { a: 2 }].should.include.something.with.property('a').something.equal(2);

@keithamus
Copy link
Member

@TueCN I'm working on getting some other things in chai handled first, but when thats done I'll be spending time with Chai Things.

@TueCN
Copy link

TueCN commented Feb 12, 2016

Great!
Thank you for your work 👍

@auni53
Copy link

auni53 commented Jun 4, 2016

+1 because this would be great

@NenadP
Copy link

NenadP commented Jun 10, 2016

+1, it would be nice if I could do

someArray.should.all.have.property('someOtherArray').that.is.an('array')

@golsan
Copy link

golsan commented Aug 26, 2016

+1

@truca
Copy link

truca commented Sep 19, 2017

@keithamus Is there any news on the subject? How could i help to solve this issue?

@keithamus
Copy link
Member

It's unlikely any of the core maintainers can get to working on chai-things any time soon. We'll likely pick it up after chaijs/chai#585

@iknowkungfoo
Copy link

I've just recently been introduced to chai and am working on unit tests related to sorting. Trying to get this to work:

expect(data).should.all.have.property('attributes');

where data looks like

{
    "data": [
        {
            "id": "8",
            "type": "some_type",
            "attributes": {
                "a": 1,
                "b": 2
            }
        },
        {
            "id": "9",
            "type": "some_type",
            "attributes": {
                "a": 3,
                "b": 4
            }
        }
    ]
}

This is where chai-things has been tripping:

// The assertion's object for `all` should be array-like
var arrayObject = utils.flag(this, "object");
expect(arrayObject).to.have.property("length");
var length = arrayObject.length;
expect(length).to.be.a("number", "all object length");

saying that length is not a number. It's actually undefined. If I make this change:

// The assertion's object for `all` should be array-like
var arrayObject = utils.flag(this, "object");
arrayObject = arrayObject.__flags.object; // This is where the data object exists.
expect(arrayObject).to.have.property("length");
var length = arrayObject.length;
expect(length).to.be.a("number", "all object length");

Then all is well with the world.

How can I verify this is the correct fix?

@llopess
Copy link

llopess commented Jun 25, 2019

+1

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

No branches or pull requests

10 participants