Skip to content
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

Cast to int the execution time #1063

Closed
wants to merge 1 commit into from
Closed

Cast to int the execution time #1063

wants to merge 1 commit into from

Conversation

goetas
Copy link
Member

@goetas goetas commented Oct 19, 2020

Cast to int the execution time (to avoid incompatibilities with RDBMS as Google Spanner)

Q A
Type bug
BC Break no
Fixed issues #1061
Alternative to #1060

->with($config->getTableName(), [
$config->getVersionColumnName() => '1230',
$config->getExecutedAtColumnName() => $executedAt,
$config->getExecutionTimeColumnName() => 31000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goetas I understand your approach. I though about the same, but, if you remove the (int) cast, the test still passes. That is why I used mock and assertSame. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the issue only happens on Google Spanner, there is no way we are going to get a failing test, since we don't have Google Spanner tested in our CI, right?

Copy link
Contributor

@gabrielfs7 gabrielfs7 Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my PR the test fails if we remove the casting, cause I am using assertSame. Here the SQLite does internal cast, different from Spanner, so the test never fails and this was already covered on other tests.

I would prefer be sure this is an integer. My test makes sure about this. I only could check this using mocks or instead I would have to refactor other classes to avoid mock, but I wanted to impact as low as possible :)

@goetas goetas force-pushed the cast-to-int-exec-time branch from 34180e1 to 42578bf Compare October 19, 2020 13:35
@goetas goetas force-pushed the cast-to-int-exec-time branch from 42578bf to b06a386 Compare October 19, 2020 13:36
@goetas
Copy link
Member Author

goetas commented Oct 19, 2020

I have changed the tests to use assertSame and willReturnCallback that should be much more safe

Copy link
Contributor

@gabrielfs7 gabrielfs7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure you can point this to my branch, but I applied your changes in the test there if you do not mind. I agree the test is cleaner. Thanks

#1060

@goetas
Copy link
Member Author

goetas commented Oct 20, 2020

Done in #1060

@goetas goetas closed this Oct 20, 2020
@goetas goetas deleted the cast-to-int-exec-time branch November 24, 2020 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants