-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore: add e2e tests for transaction summary operations #2931
Conversation
2acfe42
to
090eaec
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR closes the issue, however, the description says:
Introduces some e2e tests in the fuel-gauge test suite to ensure the operations and transaction summaries are correctly assembled for various types of transactions.
- Should we be migrating all these tests over?
- Should we remove the unit tests that have been migrated?
To the best of my knowledge, all the possible scenarios for the transaction operations are now covered so there is no further tests to be added.
This is debatable, one perspective would say that the e2e tests cover the correctness of all the unit tests, and therefore yes they should be removed. My opinion is that they should remain (for now) as they are useful for faster debugging (although the drawback is that refactors are slower) @nedsalk of course has a lot of thoughts on this. I think the ideal compromise is that delay removing them until we refactor the respective functions. |
Coverage Report:
Changed Files:
|
Summary
Introduces some e2e tests in the
fuel-gauge
test suite to ensure the operations and transaction summaries are correctly assembled for various types of transactions.Checklist