search

SolomonDefi / solomon-monorepo

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

Init e2e tests #219

Closed apolkingg8 closed 2 years ago

apolkingg8 commented 2 years ago

Close #161

Close #218

solomondefi-dev commented 2 years ago

@apolkingg8 I had to make some changes to get everything working:

  1. Bugs in api-dispute validation due to how the validator determined which event is being received. For now, I have updated the request to include the type as a parameter (/api/events/<type>, e.g. /api/events/dispute.preorder.created)
  2. Blockchain watcher testInit accepts SlmFactory directly, instead of calling Connect again. This was causing problems detecting the deployed contract
  3. Update MikroOrm to 5.1.1 - it uses a different sqlite3 package that supports arm64. I looked at the migration guide from 4 -> 5 and didn't see any obvious breaking changes that would affect our usage.
  4. Split skaffold into different configurations, so it's possible now to only run one part (npm run skaffold:backend will run API+Contracts without frontend, for example)
  5. Add supertest for testing. Raw fetch is pretty good, but supertest has a couple of nice features that I think are worth using.
apolkingg8 commented 2 years ago

@solomondefi-dev Thanks for your work. About supertest, I don't think it's a better choice because it's an additional wrap. Additional wrap means additional bugs, which is I always try to avoid. It looks like you usually like large and high-level frameworks/libraries, but I'll prefer to use fewer dependencies and design the architecture by myself. Maybe we can discuss and have a deal later, but in this case, I don't think there is a "must" reason to use it and add it directly in a code review.

solomondefi-dev commented 2 years ago

supertest is not too high level or complicated, and unlikely to be the source of bugs, it's been stable for over 7 years.

That's not why I introduced it though, there are two reasons for it:

  1. We are already using supertest, or an equivalent wrapper for tests over HTTP in other projects. It's important to be consistent with library choices and style so that developers can move between projects easily.
  2. The PR was impossible to review because many of the tests didn't pass. Some of the tests that did pass should not have, and missed some bugs in the API. I spent a few days tracking down test failures and bugs, and during that process I added supertest for consistency.

It's great to try to keep dependencies to a minimum - I appreciate that mentality and use it in my own personal projects. In team projects that need to scale in code size, it's not always the best choice.