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

Backend test setup #140

Merged
merged 21 commits into from
Dec 21, 2022
Merged

Backend test setup #140

merged 21 commits into from
Dec 21, 2022

Conversation

reginaldbondoc
Copy link
Contributor

  • Initial test configuration
  • Sample test (just the healthcheck endpoint for now)
  • Moved things from index.ts to separate parts for easier test setup
  • Initial coverage configuration
  • GH Action workflow
    • Modified Backend PR check to run test
    • New workflow to post comment/report of test result and coverage to PR (Note: this new workflow will not successfully run until merged into target main branch)

Sample:

Test result:
image

Coverage annotations:
image

Note: some files were automatically modified by prettier on commit (husky?).

@maidul98
Copy link
Collaborator

For code coverage, will the missed branch or line be shown to the user? I'm curious how they can know what has caused the coverge to fail

@reginaldbondoc
Copy link
Contributor Author

For code coverage, will the missed branch or line be shown to the user? I'm curious how they can know what has caused the coverge to fail

Yes:
image

(But the link there just points to commit, not actual file/line in GitHub). But this shall provide proper feedback on which line/branch is not covered.

@Zamion101
Copy link
Contributor

Zamion101 commented Dec 21, 2022

Will be the tests only available to run in GH CI/CD or is it possible to run test locally? Example npm run test:backend

@reginaldbondoc
Copy link
Contributor Author

Will be the tests only available to run in GH CI/CD or is it possible to run test locally? Example npm run test:backend

Possible to run test locally. Dev can do: npm test

@maidul98
Copy link
Collaborator

Lastly, i didn't see a coverge threshold defined unless there is a default? https://jestjs.io/docs/configuration#coveragethreshold-object

@dangtony98 can you also review the index.ts file to make sure you are up to date with the new restructure?

Copy link
Contributor

@Zamion101 Zamion101 left a comment

Choose a reason for hiding this comment

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

There are some questions I'm curious about. Overall not seeing any major security or design flaw. There needs to be more work on some parts towards consistent type definition and error handling. I will be working on central error handling but first I need this PR to be merged into main for me to start working on some changes.

backend/src/services/health.ts Outdated Show resolved Hide resolved
@reginaldbondoc
Copy link
Contributor Author

Lastly, i didn't see a coverge threshold defined unless there is a default? https://jestjs.io/docs/configuration#coveragethreshold-object

That's correct. Because there's no test yet, aside from the initial one that I added. If we define the threshold, the run will fail. 😉

For now, I plan to just start with this, and add more tests as we move forward. Once we reach 50%, I would suggest we set the threshold and then aim higher. 👍

@reginaldbondoc
Copy link
Contributor Author

I will be working on central error handling

❤️

first I need this PR to be merged into main for me to start working on some changes

That's consistent with my rationale of opening this PR as early as now 👍

Copy link
Contributor

@Zamion101 Zamion101 left a comment

Choose a reason for hiding this comment

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

I'm okay with current state of this code. Waiting for your review @dangtony98

@maidul98 maidul98 merged commit 2d3255e into Infisical:main Dec 21, 2022
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.

3 participants