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

HPC-8232, HPC-7811, HPC-8130 - setup testing environment with jest and docker #26

Merged
merged 10 commits into from
Feb 29, 2024

Conversation

ashwinkjoseph
Copy link

@ashwinkjoseph ashwinkjoseph commented Oct 31, 2021

Set up unit testing using Jest and Docker. In Docker compose, empty Postgres database is initialized using a minimal schema that matches what’s currently being used in HPC. The schema was generated from Postgres container from hpc-suite's start.sh with latest production DB synced to local:

docker exec hpc_service-pgsql-1 pg_dump --schema-only --no-owner -U postgres hpc --file /backups/schema-2024-02-29.sql

A few example tests were added and those which interact with models are using transactions.

This PR is related to tickets HPC-8232, HPC-7811 and HPC-8130.

@ashwinkjoseph ashwinkjoseph requested a review from a team as a code owner October 31, 2021 13:54
@ashwinkjoseph ashwinkjoseph requested review from Pl217 and s0 October 31, 2021 13:55
@ashwinkjoseph
Copy link
Author

ashwinkjoseph commented Oct 31, 2021

The CI checks seems to be failing for src/util/async.ts as per https://github.com/UN-OCHA/hpc-api-core/runs/4060326553?check_suite_focus=true which isn't a file modiified in this PR. Has a PR been merged without the CI checks passing before? (I did a rebase after committing my changes)

This has been fixed in #27

@ashwinkjoseph ashwinkjoseph mentioned this pull request Oct 31, 2021
@ashwinkjoseph
Copy link
Author

ashwinkjoseph commented Oct 31, 2021

The CI checks seems to be failing for src/util/async.ts as per https://github.com/UN-OCHA/hpc-api-core/runs/4060326553?check_suite_focus=true which isn't a file modiified in this PR. Has a PR been merged without the CI checks passing before? (I did a rebase)

This has been fixed in #27

Need to add prefix to container names in docker compose to avoid conflict, however can't add another commit without this issue being fixed due to commit hooks. So please merge #27 as soon as you can and wait for another commit from me on #26 to avoid developers facing conflict with their existing containers

@Pl217
Copy link
Contributor

Pl217 commented Nov 1, 2021

The CI checks seems to be failing for src/util/async.ts as per https://github.com/UN-OCHA/hpc-api-core/runs/4060326553?check_suite_focus=true which isn't a file modified in this PR. Has a PR been merged without the CI checks passing before? (I did a rebase after committing my changes)

This has been fixed in #27

No PRs are merged without CI passing.

If you take a look at .github/workflows/ci.yml file in this repo, you will see that we install packages with yarn and --frozen-lockfile flag, which means package installation should strictly follow yarn.lock package versions.

The real reason this PR is failing is because you overwritten this lock file when installing packages and you committed the changes. So, after adding jest, ts-jest and @types/jest to package.json, you should just use yarn command and lock file would be updated. I don't know why you downgraded lint-staged and prettier.

Next, in ci.yml, after installing packages with respect of lock file, we run npm run check-types (corresponding to tsc --noEmit) and npm run lint (corresponding to prettier -c . && eslint --quiet .). As you can see, entire codebase is checked for Typescript types, as well as prettier formatting and eslint linting.
That is why you see the error even though you haven't changed the file.

Now, we come to the reason why the CI fails. Since Typescript 4.4 was released, it introduced a change to make all errors in catch blocks default to unknown type and not any type.

So, since you changed yarn.lock file to install Typescript v4.4.4, we see this error failing even though it wasn't in the past. If yarn.lock was followed from develop, we'd have Typescript v4.3.5 installed and type checks are passing.

Lastly, as a proof that CI in PRs was passing (and that this PR introduced changes which cause CI to fail), look at #25.

Need to add prefix to container names in docker compose to avoid conflict, however can't add another commit without this issue being fixed due to commit hooks. So please merge #27 as soon as you can and wait for another commit from me on #26 to avoid developers facing conflict with their existing containers

I have explained the reasons for failing CI in lots of detail above. If you need to commit and commit hooks stand in your way, just use git commit -n.

For this PR in particular, I would say:

  • Edit package.json not to downgrade prettier and lint-staged packages
  • Restore yarn.lock to state on develop: git checkout origin/develop yarn.lock
  • Delete node_modules (sudo rm -rf node_modules/) and do yarn or yarn install
  • Amend the commit with git commit --amend --no-edit

You could also split this work across multiple commits, but keep the packages in yarn.lock which shouldn't have their minor or patch version updated.

@ashwinkjoseph
Copy link
Author

Okay

The CI checks seems to be failing for src/util/async.ts as per https://github.com/UN-OCHA/hpc-api-core/runs/4060326553?check_suite_focus=true which isn't a file modified in this PR. Has a PR been merged without the CI checks passing before? (I did a rebase after committing my changes)
This has been fixed in #27

No PRs are merged without CI passing.

If you take a look at .github/workflows/ci.yml file in this repo, you will see that we install packages with yarn and --frozen-lockfile flag, which means package installation should strictly follow yarn.lock package versions.

The real reason this PR is failing is because you overwritten this lock file when installing packages and you committed the changes. So, after adding jest, ts-jest and @types/jest to package.json, you should just use yarn command and lock file would be updated. I don't know why you downgraded lint-staged and prettier.

Next, in ci.yml, after installing packages with respect of lock file, we run npm run check-types (corresponding to tsc --noEmit) and npm run lint (corresponding to prettier -c . && eslint --quiet .). As you can see, entire codebase is checked for Typescript types, as well as prettier formatting and eslint linting. That is why you see the error even though you haven't changed the file.

Now, we come to the reason why the CI fails. Since Typescript 4.4 was released, it introduced a change to make all errors in catch blocks default to unknown type and not any type.

So, since you changed yarn.lock file to install Typescript v4.4.4, we see this error failing even though it wasn't in the past. If yarn.lock was followed from develop, we'd have Typescript v4.3.5 installed and type checks are passing.

Lastly, as a proof that CI in PRs was passing (and that this PR introduced changes which cause CI to fail), look at #25.

Need to add prefix to container names in docker compose to avoid conflict, however can't add another commit without this issue being fixed due to commit hooks. So please merge #27 as soon as you can and wait for another commit from me on #26 to avoid developers facing conflict with their existing containers

I have explained the reasons for failing CI in lots of detail above. If you need to commit and commit hooks stand in your way, just use git commit -n.

For this PR in particular, I would say:

  • Edit package.json not to downgrade prettier and lint-staged packages
  • Restore yarn.lock to state on develop: git checkout origin/develop yarn.lock
  • Delete node_modules (sudo rm -rf node_modules/) and do yarn or yarn install
  • Amend the commit with git commit --amend --no-edit

You could also split this work across multiple commits, but keep the packages in yarn.lock which shouldn't have their minor or patch version updated.

Okay got it, I had a merge conflict on my yarn.lock so I did a fresh yarn.install. So I haven't actually downgraded anything in package.json.
I think the best approach to follow here would be to remove node_modules and restore yarn.lock and package.json to the version on develop and then add the packages I need using yarn add

@Pl217
Copy link
Contributor

Pl217 commented Nov 1, 2021

Okay got it, I had a merge conflict on my yarn.lock so I did a fresh yarn.install.

I assume you made changes, committed and then rebased on latest develop, which resulted in a conflict.

So I haven't actually downgraded anything in package.json. I think the best approach to follow here would be to remove node_modules and restore yarn.lock and package.json to the version on develop and then add the packages I need using yarn add

Yup, this is the way I would do it. You can use the commands I put above.

@s0 s0 added needs minor changes There are review or issue comments to address ready for review All comments have been addressed, and the Pull Request is ready for review labels Nov 10, 2021
@s0 s0 removed their assignment Dec 8, 2021
@Pl217 Pl217 assigned manelcecs and unassigned ashwinkjoseph Sep 13, 2023
@manelcecs manelcecs removed the ready for review All comments have been addressed, and the Pull Request is ready for review label Sep 22, 2023
@manelcecs manelcecs added ready for review All comments have been addressed, and the Pull Request is ready for review and removed needs minor changes There are review or issue comments to address labels Oct 26, 2023
@manelcecs manelcecs assigned Pl217 and unassigned manelcecs Oct 26, 2023
@Pl217 Pl217 merged commit 9fab873 into develop Feb 29, 2024
2 checks passed
@Pl217 Pl217 deleted the tests branch February 29, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review All comments have been addressed, and the Pull Request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants