search

veres-one / did-veres-one

A Decentralized Identifier utility library for Veres One
BSD 3-Clause "New" or "Revised" License
9 stars 4 forks source link

Fix fingerprint #22

Closed mattcollier closed 5 years ago

mattcollier commented 5 years ago

See changelog.

Depends on: https://github.com/digitalbazaar/crypto-ld/pull/13

dmitrizagidulin commented 5 years ago

👍 from me, except we need to put dirty-chai and expect().to.exist() back into the tests.

dmitrizagidulin commented 5 years ago

To say more, re dirty-chai - take a look at the Why? section of its readme.

mattcollier commented 5 years ago

dirty-chai is not happening. If there are indeed issues with regular chai, please provide a minimal runnable example of the issue so we can debug.

The issue I encountered was that if one uses regular chai syntax (eg. to.exiist or to.be.true) with dirty-chai installed those assertions do not throw.

We use regular chai throughout bedrock and will not be using dirty-chai here.

On Tue, Jan 29, 2019 at 9:20 PM Dmitri Zagidulin notifications@github.com wrote:

👍 from me, except we need to put dirty-chai and expect().to.exist() back into the tests.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/veres-one/did-veres-one/pull/22#issuecomment-458785949, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3DmT4zE9fGhWJo3VT7_ub0oTtb_0lZks5vIQF4gaJpZM4aZDHa .

-- Matt Collier

dmitrizagidulin commented 5 years ago

Ok, but, you should also use dirty chai throughout bedrock. You're setting yourself up for false positives all over the place

dmitrizagidulin commented 5 years ago

Also, why would using dirty-chai here affect your bedrock tests in any way?

dmitrizagidulin commented 5 years ago

Chai should be filthy, I say.

mattcollier commented 5 years ago

@dmitrizagidulin I did not see anything compelling in the why section of the dirty-chai readme. Can you demonstrate a problem that dirty-chai solves?

And with ~300 modules and more being created weekly, devs should not have to concern themselves about which assertion library is being used. Mocha/chai has been serving us very well.

dmitrizagidulin commented 5 years ago

Sure. To demonstrate why regular Chai assertions are dangerous, try this test:

  it.only('should fail', () => {
    expect(v1).to.whatever;
  });

(You can like, insert it into VeresOne.spec.js and run npm test).

.whatever is not a valid property. But the test will still pass.

This is talked about more here, also: Beware of libraries that assert on property access

mattcollier commented 5 years ago

@dmitrizagidulin unfortunately that is exactly why I ripped out dirty-chai with fire and fury. If one makes a legitimate assertion using regular chai syntax with dirty chai installed expect(undefined).to.exist; does not throw. So dirty-chai does not prevent devs from creating invalid assertions.

dmitrizagidulin commented 5 years ago

@mattcollier Wait though. It doesn't throw even without dirty-chai installed. The point is, regular chai is fundamentally broken.

dmitrizagidulin commented 5 years ago

To put it another way - the behavior you think dirty-chai is causing? Is caused by plain chai.

mattcollier commented 5 years ago

@dmitrizagidulin to ensure that we are talking about the same thing. If you change this line from to.be.false to to.be.true you will see that the assertion throws as expected: https://github.com/veres-one/did-veres-one/blob/bc7fb3412fb99fd5216f22d5cb114ec4cb3e4ccb/tests/VeresOneDidDoc.spec.js#L288-L289

dmitrizagidulin commented 5 years ago

@mattcollier Yes. But if you change it from .to.be.false to .to.be.somethinginvalid, it still passes. And that's without dirty-chai.

mattcollier commented 5 years ago

I speculate that you may have in the past fallen victim to the same issue I encountered today. Which was that if dirty-chai is required and chai.use(dirtyChai); is called in any of the spec files in the test suite, dirty-chai will be in effect in spec files that make no reference at all to dirty-chai. So if one removes dirty-chai only from a single spec file, the behavior related to regular chai syntax not throwing will occur.

mattcollier commented 5 years ago

@mattcollier Yes. But if you change it from .to.be.false to .to.be.somethinginvalid, it still passes. And that's without dirty-chai.

That's with and without dirty-chai which is my point. Dirty-chai doesn't solve the problem. IMO it makes it worse because it disables valid chai syntax. Dirty-chai does not prevent devs from writing bad assertions.

dmitrizagidulin commented 5 years ago

I think the idea is not that it prevents devs from writing bad assertions. It's that if devs only use assertions that end in (), those will fail more consistently.

dmitrizagidulin commented 5 years ago

Also, dirty chai enables IDE based linting to do its job properly

dmitrizagidulin commented 5 years ago

Alllll of that said, I think you feel way more strongly about not having dirty-chai. And I can certainly live without it. :) So, +1 from me without reservations, on this PR.

mattcollier commented 5 years ago

@dmitrizagidulin @dlongley ready for another pass with new terminology: https://github.com/veres-one/did-veres-one/pull/22/commits/0be8a78017d1bf5c8d0de0c7aa706170feac052c