search

opensearch-project / dashboards-observability

Visualize and explore your logs, traces and metrics data in OpenSearch Dashboards
https://opensearch.org/docs/latest/observability-plugin/index/
Apache License 2.0
17 stars 56 forks source link

[FEATURE] Run Typescript Check in CI #676

Open derek-ho opened 1 year ago

derek-ho commented 1 year ago

Observability Dashboards 2.9 broke the release candidate because the following:

Diagnostics: Property 'forEach' does not exist on type '"application_analytics" | "operational_panels" | "event_analytics" | "notebooks" | "trace_analytics" | "metrics_analytics" | "integrations"'. Property 'forEach' does not exist on type '"application_analytics"'. [2339]

We should fail on any typescript errors, to avoid this in the future.

ps48 commented 1 year ago

Linking PRs that were close to release timelines: 2.9: https://github.com/opensearch-project/dashboards-observability/pull/672 2.5 https://github.com/opensearch-project/dashboards-observability/pull/192

@derek-ho This can be as simple as using the TS action in CI: https://github.com/marketplace/actions/typescript-error-reporter

@joshuali925 On a side note, if there a way to mock API calls on jest or create a Cypress test to check metrics are updated on actual API usages. This can help us check Dashboards never has NPE/runtime errors again.

Swiddis commented 1 year ago

Adding it could be a hefty refactor, because there's a lot of code I've seen in the current codebase that is failing in typescript. A quick local run yields 3020 TS errors across 752 files.

derek-ho commented 1 year ago

@Swiddis I think we should apply it to all files in server/* for example to start with - this is where most issues occur anyways, other changes should be caught during local dev mostly, but we can slowly add those paths in

Swiddis commented 1 year ago

Did more poking around and it looks like a lot of the typescript errors come from OSD. Tried to look for if there's a way to filter it out but I don't see anything, someone online rightfully points out that if the dependency is broken, it's very difficult to ensure that the dependent project is not broken. Will need to do some more research on our options when I have time.

joshuali925 commented 1 year ago

@joshuali925 On a side note, if there a way to mock API calls on jest or create a Cypress test to check metrics are updated on actual API usages. This can help us check Dashboards never has NPE/runtime errors again.

@ps48 yes we can probably add something for it. although for tests to be able to catch https://github.com/opensearch-project/dashboards-observability/pull/192 might not be trivial, i haven't looked into how to hit a mock router

Swiddis commented 12 months ago

Also seems related to #11