Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Run Typescript Check in CI #676

Open
derek-ho opened this issue Jul 14, 2023 · 6 comments
Open

[FEATURE] Run Typescript Check in CI #676

derek-ho opened this issue Jul 14, 2023 · 6 comments

Comments

@derek-ho
Copy link
Collaborator

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.

@derek-ho derek-ho added enhancement New feature or request untriaged and removed untriaged labels Jul 14, 2023
@ps48
Copy link
Member

ps48 commented Jul 14, 2023

Linking PRs that were close to release timelines:
2.9: #672
2.5 #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
Copy link
Collaborator

Swiddis commented Jul 14, 2023

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

Swiddis commented Jul 14, 2023

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
Copy link
Member

@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 #192 might not be trivial, i haven't looked into how to hit a mock router

@Swiddis
Copy link
Collaborator

Swiddis commented Dec 6, 2023

Also seems related to #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants