[Part 4|*] Add unit tests for the previous introduced SQL Backend methods #10292
Conversation
ziggie1984
changed the base branch from
master
to
elle-payment-sql-series-new
ziggie1984
changed the title
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
3 times, most recently
from
415144d to
439b619
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
from
439b619 to
c922bc3
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
7 times, most recently
from
3648e20 to
ae6c366
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive unit tests for the newly introduced SQL backend for the payments database. The changes include refactoring existing tests to be database-agnostic, which is a great improvement for maintainability. The new SQL queries and the Go store implementation are well-structured and cover a wide range of functionalities. My main feedback is on adhering to the testing style guide in the new TestQueryPayments function, where t.Fatalf and t.Errorf should be replaced with assertions from the require library as mandated by the repository's style guide.
payments/db/payment_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("unable to create test "+ | ||
| "payment: %v", err) | ||
| } |
There was a problem hiding this comment.
The project's style guide specifies that unit tests must use the require library for assertions.1 Please replace if err != nil { t.Fatalf(...) } with require.NoError(t, err, ...) for consistency and conciseness. This also applies to other t.Fatalf and t.Errorf calls in this test function, which should be replaced with appropriate require assertions (e.g., require.Equal, require.Len).
require.NoError(t, err, "unable to create test payment")Style Guide References
Footnotes
-
The style guide mandates the use of the
requirelibrary for assertions in unit tests to ensure consistency and readability. ↩
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
from
ae6c366 to
b011ea6
Compare
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
2 times, most recently
from
b515aaa to
bdbcc54
Compare
We wrap the fetchPayment db call and catch the case where no errors are found in the db, where we now return the ErrPaymentNotInitiated error.
|
@ziggie1984, remember to re-request review from reviewers when ready |
Now that every method of the interface was implemented we can remove the embedded reference we put into place for the sql store implementation so that the interface would succeed. This is now removed.
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
from
20ab16c to
17c5490
Compare
Since now the sql backend is more strict in using the same session key we refactor the helper so that we can easily change the session key for every new attempt.
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
2 times, most recently
from
e367d2e to
3140602
Compare
We make the QueryPayments test db agnostic and also keep a small test for querying the duplicate payments case in the kv world.
In commit adds the harness which will be used to run db agnostic tests against the kv and sql backend. We have adopted all the unit tests so far so that with this commit all the payment tests not specifically put into the kv_store_test.go should all pass for all backends.
The design of the sql and kv db are a bit different. A harness interface is introduced which allows us to unit most of the test and keep the backend specific tests at a minimum.
ziggie1984
force-pushed
the
introduce-sql-schema-payments-part-4
branch
from
3140602 to
4cb478e
Compare
2 participants
This PR adds the DB related unit tests for backend functions which were introduced in #10287,#10291 and #10368
and finalizes the implementation for the SQL backend.
This PR now lets the unit tests and the itest pass for the SQL payments backend.