pharos icon indicating copy to clipboard operation
pharos copied to clipboard

Unit test assertions about errors always pass

Open brentswisher opened this issue 1 year ago • 4 comments

Expected behavior The unit tests would fail when invalid assertions are made about the errors being thrown

Actual behavior The unit tests pass, even when making invalid assertions about the errors that are being thrown

Steps to reproduce the issue

  1. Download Pharos Locally
  2. Open the pharos icon tests file
  3. Change the "throws an error for an invalid icon name" assertion to be something silly:
  it('throws an error for an invalid icon name', async () => {
    component = await fixture(html` <test-pharos-icon name="fake"></test-pharos-icon> `).catch(
      (e) => e
    );
    expect('Could not find the moon').to.be.thrown;
  });
  1. Run tests using yarn test
  2. See that the tests still pass

Pharos version 12.21.0 - latest develop branch

Your environment

  • OS: macOS
  • Browser: N/A
  • Version: N/A

Additional information It looks like this is just the wrong syntax when using chai, and it should be something like

expect(badFn).to.throw('salmon');

While I found this in the pharos-icon component, a similar syntax is used in 18 other places in the library and should probably all be fixed

/pharos/packages/pharos/src/components/alert/pharos-alert.test.ts
  25,47:     expect('status is a required attribute.').to.be.thrown;

/pharos/packages/pharos/src/components/button/pharos-button.test.ts
  141,82:       expect('fake is not a valid type. Valid types are: button, submit, reset').to.be.thrown;

/pharos/packages/pharos/src/components/heading/pharos-heading.test.ts
  43,46:     expect('level is a required attribute.').to.be.thrown;
  50,82:     expect('7 is not a valid heading level. Valid levels are: 1, 2, 3, 4, 5, 6').to.be.thrown;
  59,7:     ).to.be.thrown;

/pharos/packages/pharos/src/components/icon/pharos-icon.test.ts
  21,39:     expect('Could not find the moon').to.be.thrown;

/pharos/packages/pharos/src/components/image-card/pharos-image-card.test.ts
  137,81:     expect('fake is not a valid variant. Valid variants are: base, collection').to.be.thrown;
  151,7:     ).to.be.thrown;
  165,7:     ).to.be.thrown;

/pharos/packages/pharos/src/components/layout/pharos-layout.test.ts
  58,7:     ).to.be.thrown;

/pharos/packages/pharos/src/components/modal/pharos-modal.test.ts
  372,79:     expect('fake is not a valid size. Valid sizes are: small, medium, large').to.be.thrown;

/pharos/packages/pharos/src/components/text-input/pharos-text-input.test.ts
  172,7:     ).to.be.thrown;

/pharos/packages/pharos/src/components/textarea/pharos-textarea.test.ts
  96,8:       .to.be.thrown;
  114,76:     expect('blah is not a valid wrap value. Valid values are: soft, hard').to.be.thrown;

/pharos/packages/pharos/src/components/toast/pharos-toast.test.ts
  26,78:     expect('fake is not a valid status. Valid statuses are: success, error').to.be.thrown;

/pharos/packages/pharos/src/components/toggle-button-group/pharos-toggle-button.test.ts
  33,7:     ).to.be.thrown;

/pharos/packages/pharos/src/components/tooltip/pharos-tooltip.test.ts
  124,7:     ).to.be.thrown;
  181,7:     ).to.be.thrown;

brentswisher avatar Aug 10 '23 15:08 brentswisher

Looked into this a little bit more, it seems like it is because fixture() is returning a promise, so chai is seeing a failed promise but not an actual error. It looks like https://github.com/domenic/chai-as-promised/ was created to extend chai and handle this but since we are using chai through @open-wc/testing' I'm not sure how to extent it like that.

I'll look into it more later, but wanted to share thoughts so far

brentswisher avatar Aug 10 '23 17:08 brentswisher

documentation on how to test open-wc component initialization https://open-wc.org/guides/knowledge/testing/initialization/

chrisjbrown avatar Aug 10 '23 17:08 chrisjbrown

nice find Brent. there is a closed issue in open-wc relating to chai-as-promised

https://github.com/open-wc/open-wc/issues/2284

chrisjbrown avatar Aug 10 '23 17:08 chrisjbrown

@chrisjbrown That looks promising, but running into some dependency conflicts installing it, will take another look at it later

brentswisher avatar Aug 10 '23 18:08 brentswisher