Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2824 +/- ##
==========================================
+ Coverage 72.18% 72.20% +0.01%
==========================================
Files 134 134
Lines 7449 7454 +5
Branches 1647 1555 -92
==========================================
+ Hits 5377 5382 +5
+ Misses 2026 1944 -82
- Partials 46 128 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Incogdino
left a comment
There was a problem hiding this comment.
LGTM the pretest script works as expected
|
I'm thinking about the performance implications of adding adding a setup in the pretest
-> Our CI currently runs markbind/.github/workflows/ci.yml Lines 37 to 38 in 77a3dc0
One option is to remove the Wdyt? |
|
Could removing the On a sidenote, I experimented using bun and Vite (instead of npm and webpack) for MarkBind. I left my agent on it to do the migration and it seemed like quite a bit of work to migrate our toolchain over. The main work comes from making the repo typescript and ESM native, and converting jest to the bun built-in test suite. However, the performance improvements seem very promising |
I think we could do that if we do merge in the pretest. Btw, what about the scenario where I only want to run
That sounds good, it definitely is worth exploring and feasible implementing once maintenance work is done, looking forward to see what's possible! |
|
I think moving to bun would probably come after the TypeScript migration is complete. From my preliminary (and not very scientific) testings, it seems to deliver a 3x improvement in speed when running tests and significantly faster install sleeps. |
are you suggesting to move the pretest script to the individual packages? |
Don't really have any suggestions, was just wondering 😆 I suppose it would be hard to add the pretest script into individual packages, maybe we don't have to do that hmm |
|
Just so that we are on the same page, is the concern we wont be running the pretest if we ran package specific test-scripts? I guess that would be alright because the error will be caught by the pre-push hook. |
…d into chore/update-test-script
What is the purpose of this pull request?
Overview of changes:
Whenever there are changes to dependencies or other files, some of the artifacts from tsc build or test script will
be stale and need to be regenerated. Therefore, sometimes it may past tests locally but fail in CI due to stale state
or fail tests locally after pulling changes from remote. This PR adds a pre-test cleanup step to delete those artifacts
and make sure that tests are run in a clean state.
Anything you'd like to highlight/discuss:
Testing instructions:
run
npm run testand check the logs thatnpm run setupis correctly ran and verify that tests pass as expected.Proposed commit message: (wrap lines at 72 characters)
Add pre-test setup step to clean stale artifacts
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):