Skip to content

Conversation

@berupp
Copy link

@berupp berupp commented Sep 25, 2019

Trying to address this issue.

I added two tests in the first change set:

  • TestCustomValueConverterQueryScan is mocking Query/Scan using a custom driver.ValueConverter as described here
  • TestCustomValueConverterExec is attempting to mock an insert using Exec

TestCustomValueConverterExec fails due to driver.IsValue(darg)-check, which basically filters all non-default types.
I think it is safe to remove this check, and the following reflect.DeepEquals() will successfully handle List and Map data types that are more common these days.

@codecov-io
Copy link

codecov-io commented Sep 25, 2019

Codecov Report

Merging #195 into master will increase coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   90.87%   91.11%   +0.24%     
==========================================
  Files          15       15              
  Lines         745      743       -2     
==========================================
  Hits          677      677              
+ Misses         49       48       -1     
+ Partials       19       18       -1
Impacted Files Coverage Δ
expectations_go18.go 86.66% <ø> (+5.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e98392b...181c3c5. Read the comment docs.

@berupp
Copy link
Author

berupp commented Sep 25, 2019

Note: I changed the Exec(...) fix to be supported by 1.9+ due to driver.NamedValueChecker support being introduced in 1.9 and that seems required for the registered custom driver.ValueConverter to work

@l3pp4rd l3pp4rd merged commit e64ef33 into DATA-DOG:master Sep 27, 2019
@l3pp4rd
Copy link
Member

l3pp4rd commented Sep 27, 2019

thanks, good work

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