meta: unify attributesToSQL test and type attributesToSQL and attributeToSQL #15533
Conversation
| }); | ||
| }); | ||
|
|
||
| // TODO: check what this test is supposed to do and update the possible defaultValue accordingly |
There was a problem hiding this comment.
This test is broken and should be split into multiple tests:
- One that uses
DataTypes.ARRAY(on dialects that support this type) with an empty array: should work - One that uses
DataTypes.BLOB: should never work,[]is not a valid value on a blob - One that uses
DataTypes.JSON(on dialects that support this type) : should always work - One that uses any string type (like
'JSON'insteadDataTypes.JSON): should never work because can't guess the type of an empty array and string types don't give any information
Note that the data type is an actual data type and not a string in the above two tests
| }); | ||
|
|
||
| // TODO: this test is broken for at least DB2. It adds the default value because the data type is a string, which can't have special behaviors. | ||
| // change type to an actual Sequelize DataType & re-enable |
There was a problem hiding this comment.
In this instance it's honestly default that's broken. String types don't give any information so the default value should always be added.
If DataTypes.BLOB were used, then the dialect could enforce specific behaviors
| }); | ||
| }); | ||
|
|
||
| // TODO: check what this test is supposed to do and update the possible defaultValue accordingly |
There was a problem hiding this comment.
Same remarks as the above three review notes:
Because the type is a string instead of an instanceof DataType, no special behavior can occur. We use string data types as-is, it's the equivalent of using a literal()
This test should use DataTypes.TEXT instead
An array is also not a good type to use to test that the default value is not added: A string would be a better value
| }); | ||
| }); | ||
|
|
||
| // TODO: check what this test is supposed to do and update the possible defaultValue accordingly |
| }); | ||
|
|
||
| // TODO: check what this test is supposed to do and update the possible defaultValue accordingly | ||
| it(`No Default value for JSON allowed for some dialects`, () => { |
|
Superseded by and linked in #17121 |
Pull Request Checklist
Description Of Change
Just like #15526 this PR is smaller than the others. I just want to get rid of the dialect specific QueryGenerator unit tests and doing it properly just gets me in a too big of a rabbit hole. Future PRs should migrate
attributesToSQLandattributeToSQLand overhaul the test suite.