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

chore(repo): add Vitest into codebase #57

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

HasithDeAlwis
Copy link
Collaborator

Describe The Outcome

  • Added scaffolding of Vitest into codebase, this lays down foundation for creating new unit-tests with Vitest throughout our code base
  • Added a workflow file to run all unit-tests on each pull-request

Dependency Changes

  • vitest v2.0.5 installed

Context

Essential for following Test Driven Development philosophies.

closes #44

Copy link

netlify bot commented Aug 12, 2024

Deploy Preview for cuhacking-portal-test-deployment ready!

Name Link
🔨 Latest commit 03a5c12
🔍 Latest deploy log https://app.netlify.com/sites/cuhacking-portal-test-deployment/deploys/66bad155ab37060008271a24
😎 Deploy Preview https://deploy-preview-57--cuhacking-portal-test-deployment.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -8,5 +8,6 @@ export default antfu(
},
{
...playwright.configs['flat/recommended'],
ignores: ['tests/unit-tests/**/*.ts'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm aware this is causing a build failure. Let me know how to go about this if you have any advice. Currently looking into a solution.

Copy link
Member

@MFarabi619 MFarabi619 Aug 12, 2024

Choose a reason for hiding this comment

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

image

This property doesn't exist for eslint-plugin-playwright.

Were the playwright ESLint rules causing issues with Vitest tests?

Also never push commits or make changes that break builds or tests into a PR, use a draft PR instead if you need discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2024-08-12 at 6 47 59 PM ESLint misidentifies the Vitest code as playwright

Copy link
Member

Choose a reason for hiding this comment

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

Yep working on fixing it currently

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't fix it, commented it out altogether for now. The E2E tests will be in a separate directory in the monorepo so we won't have conflicting configs anyways. Monorepo management tools handle a lot of these config headaches for you.

@HasithDeAlwis HasithDeAlwis self-assigned this Aug 12, 2024
@MFarabi619
Copy link
Member

MFarabi619 commented Aug 12, 2024

The branch for this is currently hasith/44-test-driven... which is good, I can tell who made it and what issue it was for. To make it better you can do hasith/test/44-test-driven... or hasith/chore/44-test-driven... since it was just scaffolding Vitest rather than actually writing tests. The branching guidelines are mentioned here in the docs. Feel free to add to or change it if you think it could be clearer.

Sometimes which type to apply can be ambiguous, so use your best judgement, can always be changed later. If it's getting ambiguous, it's also a good sign to split the issue into separate smaller issues to separate concerns. The issue title should specify both the branch name and the PR title.

The issue title should also be in imperative tense, so chore: Scaffold Vitest ..... And then your branch would be hasith/chore/44-scaffold-vitest... and the PR title would be the same as the issue title, chore: Scaffold Vitest .... See how much easier that makes things?

Just like there's a commit history....
image

There's also branching history....
image

Adhering to conventional spec also allows you to switch between branches easier when multi-tasking, so you can take better advantage of the distributed nature of Git. You know whose branch it is and can contact them, know the category of work to have better context, and also what the specific task is.

People looking at the list of issues can also better make out what to do. So for example this:
image

Could be [ADR] Add Issue Creation Feature to Github-Discord Webhook to Improve Communication

Since it's not a huge task, it could just be a feature issue instead. ADRs are for like big project decisions like what tech to use, what philosophy to adapt, etc.

No need to change the branch for this time. Just a note for the future, this comment can also be sent as a link to others in case this happens again.

@HasithDeAlwis
Copy link
Collaborator Author

Thanks for the advice on this, just completely missed my brain to add the spec, nothing wrong with the docs on that one. I renamed the other issue to be clearer 👍

@MFarabi619
Copy link
Member

All good all good we're learning

@MFarabi619
Copy link
Member

MFarabi619 commented Aug 12, 2024

image

In your tests directory, you added unit-tests. This is unnecessary because it's already inside the tests directory. You can just call it unit. Later you'll have integration.

So we eventually end up the following:
image

You can see the docs goes against our system. It signifies that these are tests for the docs site but we don't know what type. Only after you open it and see pom.ts

image

You can tell it was e2e tests. This will not scale. Hence why e2e tests are split into a separate app in our upcoming monorepo architecture as they do need maintenance and can grow into a significant amount of code.

For now let's leave the docs directory. I'll rename your unit-tests to unit and force push. This is a good example of how things change as your project scales.

@MFarabi619 MFarabi619 force-pushed the hasith/44-test-driven-development-philosophy branch 3 times, most recently from d0ba7aa to 848bdcf Compare August 13, 2024 00:07
@HasithDeAlwis
Copy link
Collaborator Author

Would unit-tests and integrations also be their own app?
Really cool that we can split up our apps to have their own E2E testing.

@MFarabi619
Copy link
Member

MFarabi619 commented Aug 13, 2024

Would unit-tests and integrations also be their own app? Really cool that we can split up our apps to have their own E2E testing.

Depends on scale. Your unit and integration test suites should be co-located with your app. They generally don't take too long to run, but have the highest maintenance cost. If your unit and integration tests are big enough to warrant being separated, your app should be split up/re-architected instead.

E2E tests take a while and have the lowest maintenance cost. They should also be completely black-boxed from your main app, knowing absolutely nothing about it. Hence why it gets its own directory.

@MFarabi619 MFarabi619 enabled auto-merge (rebase) August 13, 2024 03:15
@MFarabi619 MFarabi619 changed the title Test Driven Development Philosophy chore: add Vitest into codebase Aug 13, 2024
@MFarabi619 MFarabi619 changed the title chore: add Vitest into codebase chore(repo): add Vitest into codebase Aug 13, 2024
@MFarabi619 MFarabi619 force-pushed the hasith/44-test-driven-development-philosophy branch from 848bdcf to 03a5c12 Compare August 13, 2024 03:21
run: bun install

- name: 💀 Run Unit Tests
run: bun run test:vitest
Copy link
Collaborator Author

@HasithDeAlwis HasithDeAlwis Aug 13, 2024

Choose a reason for hiding this comment

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

make sure we run test:unit here

Copy link
Member

Choose a reason for hiding this comment

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

Doing some important work and can't stash, feel free to change this and force push

Copy link
Member

@MFarabi619 MFarabi619 Aug 13, 2024

Choose a reason for hiding this comment

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

Also, you want you keep the commands generic, makes mass migrations much easier. So the script for test:unit could use Jest in the future if necessary. We don't have to change all our stuff from test:vitest to test:jest.

@JowiAoun
Copy link
Collaborator

Looks good, as well as Farabi's comments here. Awaiting permissions on the deployment, I would be happy to take a chance at fixing the pipelines.

@MFarabi619 MFarabi619 merged commit d38e6da into main Aug 13, 2024
4 of 8 checks passed
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.

[ADR] Test Driven Development Philosophy
3 participants