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

chore(sql): convert tests to use tc-mysql, unpin jooq #3832

Merged
merged 2 commits into from
Jul 1, 2019

Conversation

asher
Copy link
Contributor

@asher asher commented Jul 1, 2019

@@ -38,7 +40,7 @@ class SqlCacheSpec extends WriteableCacheSpec {
((SqlCache) cache).merge('foo', data)

then:
1 * ((SqlCache) cache).cacheMetrics.merge('test', 'foo', 1, 1, 0, 0, 2, 1, 0)
1 * ((SqlCache) cache).cacheMetrics.merge('test', 'foo', 1, 1, 0, 0, 1, 1, 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

H2 special handling resulted in an additional select statement, which is no longer the case.

@@ -95,7 +95,7 @@ abstract class CacheSpec extends Specification {
'*TEST*' | ['blaTEST', 'blaTESTbla', 'TESTbla']
'bla*' | ['blaTEST', 'blaTESTbla', 'blaPest', 'blaFEST']
'bla[TF]EST' | ['blaTEST', 'blaFEST']
'bla?EST' | ['blaTEST', 'blaFEST']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test previously passed because H2 treats regex statements as case sensitive by default. MySQL defaults to case insensitive, but that's preferable. Case insensitive search was a requested feature which cat-sql delivers.

@asher asher requested a review from robzienert July 1, 2019 20:42
@asher asher merged commit a0f7b18 into spinnaker:master Jul 1, 2019
@asher asher deleted the sqlTC branch July 1, 2019 22:59
@lukaseder
Copy link

Great to see that Testcontainers provided a better solution here. I'm curious though, what was the reason for having to "pin" jOOQ to an old release? Something that could have been avoided on the jOOQ side?

@asher
Copy link
Contributor Author

asher commented Jul 2, 2019

Great to see that Testcontainers provided a better solution here. I'm curious though, what was the reason for having to "pin" jOOQ to an old release? Something that could have been avoided on the jOOQ side?

@lukaseder Thanks for asking! The problem was primarily around interactions with H2 after jooq added support for some non standard sql extensions with H2, like ON DUPLICATE KEY UPDATE which seemed to assume running H2 in MySQL mode. H2's MySQL mode seems much buggier than native however, as well as incomplete. I don't remember all the details beyond that it didn't seem to support compound primary keys correctly. Upgrading jooq and leaving H2 in its native mode caused one set of tests to fail, enabling H2's MySQL mode caused a different set to fail. We knew we wanted to go towards Testcontainers to have greater confidence in the test coverage, so pinned jooq back to punt on digging deep into the H2 weirdness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants