-
Notifications
You must be signed in to change notification settings - Fork 150
Storage testing #303
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
Storage testing #303
Conversation
a223cf6
to
5d8f0bd
Compare
7e711c5
to
876b244
Compare
Performance SummaryComparing base bb7457a with head 876b244 on TypeScript-4.9.5 benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
2a0f0d6
to
d66dbfd
Compare
Performance SummaryComparing base bb7457a with head d66dbfd on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
d66dbfd
to
342d987
Compare
Performance SummaryComparing base bb7457a with head 342d987 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to verifying the various CLI use cases in an Actions workflow. I left a question about implications of using LFS, but otherwise the changes look good.
Thank you also for the concurrency control group updates to these workflows!
@@ -0,0 +1,3 @@ | |||
data/*.tar.gz filter=lfs diff=lfs merge=lfs -text | |||
data/*.tgz filter=lfs diff=lfs merge=lfs -text | |||
data/*.zip filter=lfs diff=lfs merge=lfs -text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not clear on when LFS is required vs not required in local dev. Does this require interacting with LFS for local dev in anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question 🤔 I think when you change things matching the patterns, you'll have to. But they're currently only used in CI, so in most cases I don't think so. But I'll try this out and otherwise open a follow-up PR updating docs around this. (An external contributor wants to address the serialization bug, so merging this will help set them up easier.)
This introduces a CLI test which runs indexing and querying on some source files. These should help catch issues like #299 earlier.
As part of this PR:
A new
test-cli
workflow job that runs index, status, and query operations on a local source directory. This fails at the moment, because the current code is broken, but the check is not required to merge.Instead of cloning a repo to test on, two archives (in LFS) with curated test cases are added to the repo. One holds several TS files in various sizes for the performance benchmark. The other holds a minimal project for the CLI test. The advnatages:
It might be nice to test the index/query flow in a proper Rust test eventually. But calling the CLI in CI is the most expedient way to do some basic CLI verification.