Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
289 stars 76 forks source link

[Tests] consider erroring tests on warnings #10376

Open maxpatiiuk opened 1 month ago

maxpatiiuk commented 1 month ago

Priority impact

p - low

Test type

E2E

Which Component(s)

*

Unstable Tests

At the moment, if console.error occurs during a component test, the test will fail:

https://github.com/Esri/calcite-design-system/blob/02e71242c57ffc3d4bf322cabb96a84bc4af0a19/packages/calcite-components/src/tests/setupTests.ts#L15

In Lumina, the default behavior is to fail not just on console.error, but also on console.warn.

That behavior is chosen because generally components should not be emitting warnings - if they are, that might mean they are used incorrectly. If the test want to explicitly test incorrect/deprecated usage, then it can do so by explicitly mocking out console.warn and asserting that a given warning was triggered.

Options:

  1. Change Calcite's setupFile to fail on console.warn too. If test is expecting console.warn to be called, it can mock it as follows:

    const originalConsoleWarn = console.warn;
    
    beforeEach(() => {
     console.warn = jest.fn();  // or vi.fn() in Vitest (codemod will take care of this, so don't worry)
    });
    
    afterEach(() => {
     console.warn = originalConsoleWarn;
    });
    
    // optionally inside the test, you can asset that console.warn was called to make sure only expected warnings were produced
    • I see several tests are emitting warnings because deprecated components are rendered, or because properties are modified in componentDidUpdate triggering another re-render. Thus, you might want to create a small test util that would encapsulate the above beforeEach/afterEach calls and reuse that between the tests that emit console warnings.
  2. Keep the current behavior and overwrite default Lumina behavior. Changing the current behavior is not a requirement for migrating - it's just an opportunity to rethink the options.

Test error, if applicable

No response

PR skipped, if applicable

#

Additional Info

No response

jcfranco commented 3 weeks ago

Blocked by #10310.