Conversation
maxn990
left a comment
maxn990
left a comment
There was a problem hiding this comment.
Nice Swar! Left a few comments, but overall looks awesome. Looks like there are some formatting issues too, so just make sure to run prettier :)
| } as unknown as SelectQueryBuilder<Order>; | ||
| beforeEach(async () => { | ||
| // Run all migrations fresh for each test | ||
| await testDataSource.runMigrations(); |
There was a problem hiding this comment.
Could we clear the data and keep the schema instead of running the migrations every time? This could be quite slow
There was a problem hiding this comment.
Ok so Dalton told me why we couldn't do this. The migration that seeds the db has migrations that run on top of it. I think it should be fine since your migrations run really fast.
package.json
Outdated
There was a problem hiding this comment.
Adding a more specific path like "apps/" could help to exclude node_modules and dist, etc since we don't need to check those every time
maxn990
left a comment
maxn990
left a comment
There was a problem hiding this comment.
niceeee looks really good!! one super small comment i missed the first time then i think we'll be ready to go
| - run: yarn install --frozen-lockfile | ||
| - run: yarn list strip-ansi string-width string-length | ||
| - run: yarn test No newline at end of file | ||
| - run: npx jest No newline at end of file |
There was a problem hiding this comment.
i think these things should be moved to jobs from services
Yurika-Kan
left a comment
Yurika-Kan
left a comment
There was a problem hiding this comment.
first swar pr review! swaresome might i say..
comments on making sure backend-tests and prettier-check runs on same yarn command to avoid any possible issues in future & updating readme!
tested locally:
- made test file with bad format -> prettier check fails
- fix format with prettier write -> prettier check succeeded!
- everything else works
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| - run: yarn install |
There was a problem hiding this comment.
since backend-tests use yarn install --frozen-lockfile lets also use that here to make sure ci & local env versions stays consistent
There was a problem hiding this comment.
i think adding a small section on CI would be nice:
@sam-schu lmk thoughts
CI (GitHub Actions)
On every push and pull request, GitHub Actions runs:
- Prettier - checks formatting
- Backend tests - Jest test suite
Local Commands
Prettier:
- run
yarn prettier:checkto verify formatting locally - run
yarn prettier:writeto fix formatting locally
Backend tests: - run 'npx jest` with test DB setup
5 participants
ℹ️ Issue
Closes 132
📝 Description
Added prettier verification in our CI/CD pipeline so it runs on pushed commits and fails when there are formatting violations, and passes when they are fixed.
✔️ Verification
Tested by running yarn prettier:check and pushing a commit that I knew had formatting violations to make sure the workflow failed. Then I commited and pushed fixes by running yarn prettier:write and made sure the workflow passed since there were no violations anymore.