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

[#81] configure unit tests with vitest #58

Merged
merged 1 commit into from
Feb 6, 2024
Merged

[#81] configure unit tests with vitest #58

merged 1 commit into from
Feb 6, 2024

Conversation

JanJaroszczak
Copy link
Contributor

@JanJaroszczak JanJaroszczak commented Jan 8, 2024

Implementation of unit tests made with Vitest.
Test were written for utils functions used in the project.

  • related issue
  • My changes generate no new warnings
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the changelog
  • I have added tests that prove my fix is effective or that my feature works

MSzalowski
MSzalowski previously approved these changes Jan 10, 2024
Copy link
Contributor

@MSzalowski MSzalowski left a comment

Choose a reason for hiding this comment

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

Provide better PR description pls (remove default).

You could also update README informing about tests (technology used & how to run them)

src/vva-fe/src/utils/tests/adaFormat.test.ts Outdated Show resolved Hide resolved
@MSzalowski MSzalowski mentioned this pull request Jan 10, 2024
4 tasks
@JanJaroszczak JanJaroszczak linked an issue Jan 27, 2024 that may be closed by this pull request
This was referenced Jan 29, 2024
@Ryun1
Copy link
Member

Ryun1 commented Jan 29, 2024

In future can we please try and keep the pull requests smaller, as per the agreed Working Conventions, keeping PRs "Well-scoped - we prefer multiple PRs, rather than a big one"

@MSzalowski
Copy link
Contributor

In future can we please try and keep the pull requests smaller, as per the agreed Working Conventions, keeping PRs "Well-scoped - we prefer multiple PRs, rather than a big one"

Good point. For example that pr could be split into:

  1. Vitest integration - just config - no other tests included.
  2. Utils tests without including changes src/vva-fe/src/utils/getGovActionId.ts and test for that file - as it provides changes to the utility itself, not just tests.
  3. src/vva-fe/src/utils/getGovActionId.ts + test for the utility.

I wonder if in such cases where multiple PRs are about the same functionality (based on the above - vitest config brings nothing without tests) - wouldn't it be better, to have one global branch that would have smaller PRs to that branch and when they all are merged that global branch would have pr (big one - but with all the already reviewed and checked changes from smaller PRs) to main - just a typical git flow - would it be acceptable @Ryun1?

MSzalowski
MSzalowski previously approved these changes Jan 30, 2024
Copy link
Contributor

@MSzalowski MSzalowski left a comment

Choose a reason for hiding this comment

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

LGTM

@MSzalowski
Copy link
Contributor

@JanJaroszczak Can you rebase that branch to develop?

Based on development process instruction.

@MSzalowski MSzalowski changed the base branch from main to develop February 5, 2024 14:24
@MSzalowski MSzalowski changed the title Unit tests with Vitest [#81] configure unit tests with vitest Feb 5, 2024
@MSzalowski
Copy link
Contributor

I changed the base branch to develop on this PR and rebased (and squashed) commits delivered by @JanJaroszczak with 8c09303

Changes are assigned to me on git, but the author is @JanJaroszczak

@adgud adgud removed their request for review February 6, 2024 09:02
@MSzalowski MSzalowski merged commit bf1834d into develop Feb 6, 2024
2 of 4 checks passed
@MSzalowski MSzalowski deleted the unit-tests branch February 16, 2024 13:19
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.

Unit tests
4 participants