-
-
Notifications
You must be signed in to change notification settings - Fork 31
Added test case proving bug with boolean columns #229
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
Conversation
c8e89d9 to
0e77852
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #229 +/- ##
=========================================
Coverage 95.41% 95.41%
Complexity 1891 1891
=========================================
Files 131 131
Lines 5273 5273
=========================================
Hits 5031 5031
Misses 242 242 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Anyway it's good to have such test case 👍 |
|
@roxblnfk Seeing your comment in the updated test case: // In case of zerofill=false and unsigned=true the size value might be resolved to 4
$this->assertTrue(\in_array($qux->getSize(), [1, 4], true));Does this mean the issue still technically exists? Do we also need to account for this when comparing? public function compare(AbstractColumn $initial): bool
{
$result = parent::compare($initial);
if ($this->type === 'varchar' || $this->type === 'varbinary') {
return $result && $this->size === $initial->size;
}
if (! $result && $this->userType === 'boolean' && $this->size === 1 && $initial->size === 4) {
return true; // Handle false positives?
}
return $result;
} |
|
I'm still thinking about this. Maybe we should keep track of when the size attribute is not defined by introspection, so we can exclude it from the comparison.
|
See #228