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

ci: Type check perf #3406

Merged
merged 7 commits into from
Sep 22, 2024
Merged

ci: Type check perf #3406

merged 7 commits into from
Sep 22, 2024

Conversation

m-shaka
Copy link
Contributor

@m-shaka m-shaka commented Sep 11, 2024

The author should do the following, if applicable

part of #3377

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

detail

The main goal is to compare type check performances between main branch and each PR.

What CI jobs I added do is:

  • generate Hono app with 200 routes
  • compile a script using the app above and take diagnostics
  • on main branch, store the result to GitHub actions cache
  • on PR, compare the current performance with the latest stored result

I think it'd be nice to post the comparison as a comment on the PR (after making sure caching is working)

question

Should I do something for Codecov?

@m-shaka m-shaka force-pushed the type-check-perf branch 2 times, most recently from f6b87ef to 63778f4 Compare September 12, 2024 13:53
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.77%. Comparing base (dfbd717) to head (c342b9b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3406   +/-   ##
=======================================
  Coverage   95.77%   95.77%           
=======================================
  Files         155      155           
  Lines        9310     9310           
  Branches     2725     2756   +31     
=======================================
  Hits         8917     8917           
  Misses        393      393           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m-shaka m-shaka force-pushed the type-check-perf branch 7 times, most recently from 832882b to 019054b Compare September 13, 2024 00:28
@m-shaka m-shaka marked this pull request as ready for review September 13, 2024 23:33
@yusukebe yusukebe changed the title Type check perf ci: Type check perf Sep 17, 2024
@yusukebe
Copy link
Member

Hi @m-shaka !

This is great! It's unsure how this will be effective and how to measure performance accurately, but starting to try using it is important.

One thing about naming. How about creating perf-measures and putting this type-check-perf as type-check? In the future, we may create other scripts for measuring performance like perf-measures/routers.

@yusukebe
Copy link
Member

Should I do something for Codecov?

I'll investigate it, but it can be done after merging this.

@m-shaka
Copy link
Contributor Author

m-shaka commented Sep 17, 2024

Thank you for the feedback!
Could you explain more about what kind of files you are planning to place in type-check and perf-measures respectively?

@yusukebe
Copy link
Member

Could you explain more about what kind of files you are planning to place in type-check and perf-measures respectively?

We may not do that, and we don't have to do that, but I was thinking:

@exoego
Copy link
Contributor

exoego commented Sep 19, 2024

We can ignore coverage for type perf code by adding

type-check-perf/**/*.ts

to

hono/vitest.config.ts

Lines 19 to 30 in dfbd717

exclude: [
...(configDefaults.coverage.exclude ?? []),
'benchmarks',
'runtime-tests',
'build.ts',
'src/test-utils',
// types are compile-time only, so their coverage cannot be measured
'src/**/types.ts',
'src/jsx/intrinsic-elements.ts',
'src/utils/http-status.ts',
],

@yusukebe
Copy link
Member

We can ignore coverage for type perf code by adding

Yes, we should do it. Thank you for the advice.

@m-shaka
Copy link
Contributor Author

m-shaka commented Sep 22, 2024

Got it. You just mean mv type-check-perf perf-measures/type-check, don't you?

@yusukebe
Copy link
Member

@m-shaka

Got it. You just mean mv type-check-perf perf-measures/type-check, don't you?

Yes. Exactly!

@m-shaka
Copy link
Contributor Author

m-shaka commented Sep 22, 2024

@yusukebe moved the directory and made some refactoring!

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yusukebe
Copy link
Member

@m-shaka

Thanks! Let's go with this and keep improving it.

@yusukebe yusukebe merged commit 66df4e1 into honojs:main Sep 22, 2024
16 checks passed
@m-shaka m-shaka deleted the type-check-perf branch September 22, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants