search

SolomonDefi / solomon-monorepo

Monorepo containing core Solomon apps, services, libraries, and deploy config.
6 stars 3 forks source link

E2E testing #195

Closed apolkingg8 closed 2 years ago

apolkingg8 commented 2 years ago

Working on #161 Close #167 Close #202 Close #206 Close #211

apolkingg8 commented 2 years ago

@sampullman Please consider review and merge this PR, even if it's not complete yet. It's too old and contains too many issues.

sampullman commented 2 years ago

Ok, I will take a look today. Can you link the related issues in the description, so I can test the functionality? It seems like the only remaining open issue are #161 and #211, but I want to double check with you.

When a PR is ready all the commits should be squashed related to a single issue, but it might be difficult here because some work on various issues is mixed together.

apolkingg8 commented 2 years ago

Ok, I will take a look today. Can you link the related issues in the description, so I can test the functionality? It seems like the only remaining open issue are #161 and #211, but I want to double check with you.

When a PR is ready all the commits should be squashed related to a single issue, but it might be difficult here because some work on various issues is mixed together.

Thanks. Added related issues in the description. It contains many issues because many issues are found when I doing E2E testing, and they need the testing codebase (this branch) to develop and resolve the issues.

solomondefi-dev commented 2 years ago

I'm not sure it's ready to merge yet, there aren't too many changes, and I'm not able to run anything. Some notes:

  1. pnpm test:e2e fails with ERROR Cannot find target 'e2e' for project 'app-e2e'. It looks like the e2e executor was removed from app-e2e

  2. It looks like there is a typo in the events export, which causes the watcher to fail on initialization (maybe not related to this PR)

    Cannot find module '../interface/event.schema' or its corresponding type declarations.
[blockchain-watcher]   > 1 | export * from '../interface/event.schema'

    I'm also not sure exporting a json module like this will have the intended effect.

  3. I'm not sure the method for implementing events is workable:

    
import { PaymentCreatedEvent as IPaymentCreatedEvent } from '@solomon/shared/util-event'

export class PaymentCreatedEvent implements IPaymentCreatedEvent {


`PaymentCreatedEvent` (if exported correctly) is not an interface.

4. Unrelated to this PR - we should have a way of differentiating environment variables independent of code (e.g. a set of vars for dev, stg, and prod). Using k8s config map for non-secret env and Dockerfile env exports for secrets will allow us to consolidate environment handling between TS and Python apps. I'll create an issue for this tomorrow.
apolkingg8 commented 2 years ago

I don't have time to check every part now, but some of your notes are in expection because this PR is not completed yet. I suggest merging it because some commits like fix DB connect issue, document support, and some basic implement should be in master branch. I remove the nx:e2e because app-e2e is not complete yet so can cause error and block the CI check.

I'll check other parts in your notes tomorrow, just want to make sure you know what I consider.

beefho67 commented 2 years ago

I think it's risky to merge many commits in a single PR since it's not easy to trace back when something goes wrong. It's also not easy for a reviewer to review a huge PR. I would suggest that we split the PR and group them based on relevant issue. Off top of my head, we can create a branch one by one, cherry-pick commits that are related to a single issue, and create a new PR based on the branch. Repeat the process until are commits are merged. This may not be the best solution. There are several other ways we can refer to in this article. I am not sure if it's too late to split this PR, but at least, we can learn lessons as a team, and reinforce some good practices next time.

solomondefi-dev commented 2 years ago

For this one, once it's ready I will squash everything to a single commit before merging.

@jtse0 @apolkingg8 I see the checks are all passing now, is this ready? In the future, the PR should be marked as draft if work is still in progress.

apolkingg8 commented 2 years ago

For this one, once it's ready I will squash everything to a single commit before merging.

@jtse0 @apolkingg8 I see the checks are all passing now, is this ready? In the future, the PR should be marked as draft if work is still in progress.

You can merge it and I'll open a new one to continue dev.

solomondefi-dev commented 2 years ago

I have squashed and kept commit message history in the body. If necessary, individual commits can be reviewed by setting HEAD to d23b87e969d1e793e061c24471a0f3fa6d98487c

I will leave a couple of comments in the code here for future reference.