test: tweak test data for the Text type #67
Conversation
google-cla
bot
added
the
cla: yes
| metadata, | ||
| Column("id", Integer, primary_key=True, nullable=True), | ||
| Column("text_data", Text), | ||
| ) |
There was a problem hiding this comment.
The test is actually testing Text type, so I suppose we can make the id column (which is Integer and not directly considered by the test) nullable and without autoincrement (which is not supported anyway). The change makes three more tests passing (there are some cleanup/teardown errors, but the tests themselves are working fine - we'll take a look at cleanups little bit later).
There was a problem hiding this comment.
#36 does too many overrides. You only need to change several symbols to add manual primary key, but it requires to copy the whole test source code for every failing test.
Plus to this, #36 includes some unclear changes, like this one:
It looks pretty much like a meaningful diverge from the original test. Instead all of it just a single test data override can be done, keeping the original tests as they were.
larkee
left a comment
larkee
left a comment
There was a problem hiding this comment.
Please revert the changes to test class ordering. Additionally, could this logic be extended to other test classes or is it unique to TextTest?
Not sure what does it mean "to test class ordering". Yeah, I suppose there is a couple of more test cases, which could use the similar data definition override instead of overriding every test - that's what I was talking about earlier. Still, all of the PRs now merged, so it's not very obvious on which tests this logic can be extended. Requires a local revert and closer look. |
Ah, my wording was unclear. The test classes have been reordered e.g.
This looks to have been address in #75 so once the reordering has been addresses this LGTM 👍 |
|
@larkee, oh, I got it. Reverted. |
4 participants
No description provided.