[Maintenance] CI Failure Due to Typescript Bump in OSD Main Branch

[ps48]

Description

The dashboards-observability plugin currently utilizes OpenSearch-Dashboards (OSD) in its continuous integration (CI) process to build and run tests. A recent update in the OSD main branch from TypeScript 4.0.4 to 4.6.4 has caused a discrepancy in the plugin's CI, as this update was only applied to the main branch and not backported to the 2.x branch.

Unlike other dashboards plugins, dashboards-observability uses ts-jest instead of babel transform with jest. Consequently, when OSD bumped up TypeScript in the main branch, the CI for dashboards-observability failed due to snapshot mismatches. It's worth noting that all other plugin CIs, which use babel transform, continued to work as expected.

Snapshot Mismatch Example

expect(received).toMatchSnapshot()

Snapshot name: `Live tail off button stop live tail 1`

- Snapshot  - 2
+ Received  + 2

@@ -1,6 +1,6 @@
- <StopLiveButton
+ <Component
    StopLive={[MockFunction]}
    dataTestSubj=""
  >
    <EuiButton
      color="danger"
@@ -83,6 +83,6 @@
            </span>
          </EuiButtonContent>
        </button>
      </EuiButtonDisplay>
    </EuiButton>
- </StopLiveButton>
+ </Component>

  70 |     
  71 |         await waitFor(() => {
> 72 |           expect(wrapper).toMatchSnapshot();
     |                           ^
  73 |         });
  74 |       });
  75 |  });

  at public/components/common/live_tail/__tests__/live_tail_button.test.tsx:72:27

Proposed Solutions

The root cause of the CI failure is the difference in the TypeScript version used in the main branch compared to the 2.x branch of OSD. The switch to ts-jest in dashboards-observability exacerbated the issue, resulting in snapshot mismatches.
Proposed Solutions

Short-term Solution

1. Update the existing snapshot tests to align with the changes introduced by the TypeScript update in the OSD main branch.

Long-term Solution

1. Switch back to using babel transform instead of ts-jest in dashboards-observability.
2. Consider removing snapshots from testing altogether, as they might not provide significant value and could lead to similar issues in the future due to the non-semantic versioning nature of TypeScript and ts-jest.

Considerations

Since TypeScript and ts-jest don't follow strict semantic versions, it is crucial to address the potential for future issues. The proposed solutions aim to provide both short-term mitigation for the current problem and long-term strategies to prevent similar challenges. The team should carefully evaluate the trade-offs and choose the approach that best aligns with the project's goals and maintainability.

XKCD ref: https://xkcd.com/1172/

[joshuali925]

i remember there were some issues when adding babel jest, that's why i used ts-jest instead in a939098. but can't remember what exactly wasn't working. i think it makes sense to change to babel jest, there was a similar discussion in assistant opensearch-project/dashboards-assistant#3 (comment)

probably not a good idea but i also want to bring up other options like @swc/jest, which will improve CI performance if jest is running slow here

[joshuali925]

babel-jest seems ok to migrate to, broke 1 test but everything else worked

Summary of all failing tests
 FAIL  public/components/integrations/components/__tests__/create_integration_helpers.test.ts (7.194 s)
  ● doExistingDataSourceValidation › Returns no errors if everything passes

    expect(received).resolves.toHaveProperty(path, value)

    Expected path: "ok"

    Expected value: true
    Received value: false

      329 |     const result = doExistingDataSourceValidation('target', 'name', 'type', mockHttp as HttpSetup);
      330 |
    > 331 |     await expect(result).resolves.toHaveProperty('ok', true);
          |                                   ^
      332 |   });
      333 | });
      334 |

      at Object.toHaveProperty (../../node_modules/expect/build/index.js:242:22)
      at _callee10$ (public/components/integrations/components/__tests__/create_integration_helpers.test.ts:331:35)
      at tryCatch (node_modules/@babel/runtime/helpers/regeneratorRuntime.js:45:16)
      at Generator.<anonymous> (node_modules/@babel/runtime/helpers/regeneratorRuntime.js:133:17)
      at Generator.next (node_modules/@babel/runtime/helpers/regeneratorRuntime.js:74:21)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (node_modules/@babel/runtime/helpers/asyncToGenerator.js:22:9)
      at node_modules/@babel/runtime/helpers/asyncToGenerator.js:27:7
      at Object.<anonymous> (node_modules/@babel/runtime/helpers/asyncToGenerator.js:19:12)


Test Suites: 1 failed, 1 skipped, 113 passed, 114 of 115 total
Tests:       1 failed, 1 skipped, 347 passed, 349 total
Snapshots:   170 passed, 170 total
Time:        148.707 s, estimated 343 s
Ran all test suites.

i tried @swc/jest, not much performance improvement. finished in 133.627 s, but had issues with jest.spyOn and 5 tests failed.

for reference ts-jest have all tests passed and finished in 132.979 s

[mengweieric]

Add thoughts here for considering removing snapshots.

I feel there are some concerns worth discussing particular given the fact that there is ongoing campaigns to bring in enough test coverages on both unit tests and end-to-end tests.

The snapshots are very sensitive that even minor changes in components lead to snapshot test failures, which in turn, often resulting in false positives that require frequent updates. In our cases it also sometimes lead to just simple updates and checks for only getting snapshots updated without actual leverage of the failures themselves to spot and address potential issues.

Additionally, there will also be cases that we end up with large snapshots which we will face challenges with those large snapshots being hard to analyze effectively, while in the main time taking efforts to constantly maintaining them.

Given that we are current working towards getting quality assurance mechanism to an optimal state, the benefits we gain from having snapshots dropped significantly to my opinion, though there are benefits using snapshots.

[joshuali925]

Agree to @mengweieric, sometimes I want to use snapshot for schema/object validation in node.js server side, but I was worried that if anything fails, snapshots might get updated by yarn test -u without fixing the actual problem.

I think snapshots are useful but we shouldn't need to use it in every test

[pjfitzgibbons]

I halfway-agree. Snapshots do not prove coverage. Snapshots though give us an imporatant tool to detecting unintentional UI/CSS changes. While it is sometimes a pain to update and verify 100 snapshots b/c we made a sweeping change, that intentionality is super important to maintaining the UX of the app. Our pain w/ snapshots lately have been widespread refactors that expectedly change a lot of behind-the-scenes html... which we still want to be aware of.

[pjfitzgibbons]

I think snapshots are useful but we shouldn't need to use it in every test

I feel this is technically a code-smell. It's in a lot of places in our app. The snapshot is literally testing only that the component renders at all... without any other validation. This is a test smell. It means our code is in such a state that it is effectively not-understandable and not-testable. This is partly an effect of React b/c it's a complex framework (side-effects!!). Our focus should be on creating testable code, even if that means r/n adding tests to utility functions b/c those are testable.

[joshuali925]

The issues i see in snapshots are:

1. false positives. many times snapshot changed but doesn't mean there is a visual change, for example this issue. it leads to maintenance overhead
2. snapshot too large. we have snapshots about 20k lines, for example

   dashboards-observability/public/components/application_analytics/__tests__/__snapshots__/create.test.tsx.snap

   Line 19071 in fd55726

   `;

   it's not clear what the expectation is if something in it changes, as the html gets hard to understand which part corresponds to which UI and hard to determine if the changes were intentional
3. too strict. for example we can add snapshots to test css styling, but in reality every little content change would cause snapshots to fail. Since it's trivial to "fix" the snapshots, it's not very reliable to trust that what was being tested before (css styling) aren't being broken when snapshots get updated.

They are partially issues with this specific repo, but partially problems in the snapshot testing design. I can see other forums/blog posts have similar opinions, and the summary is that it's hard to tell if there's a code issue or test issue when snapshot fails.

I think one way to mitigate is to reduce the snapshot scope and only render a small component in each snapshot, and changes will be more manageable. But i'm still not sure if it's worth the maintenance effort, especially in this repo where there are a lot of expected UI changes

[ps48]

We moved to babel test as noted from the above two PRs. Closing this issue for now. We can re-open this later as needed.