-
Notifications
You must be signed in to change notification settings - Fork 17
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
add self test for test type possible_duplicate_forms #43
Conversation
note that the tests are failing since this affects the test totals (number of rows that have failed) and the failed rows themselves |
Hi @lrnzcig,
The name 'duplicate_forms' is specific to the use-case as not all data DOT will be used with relates to forms, I think we could rename this to something like 'duplicate_followup_record' or something.
Very possibly, I commented out some test types as part of hitting the submission for global digital goods. If the bug fix resolves this, then great. Note that the bug may have been introduced as part of the refactor for open source perhaps, though we did ensure self-tests were passing so that seems a bit odd. Thanks! |
thanks @dividor no, the self tests did not cover this (always knew that our test coverage was not so great, but you don't expect such a bug to come through!) the test that is failing now is a new one I added in my last PR... I don't see any other explanation that the bug being there all along... but there is a very clear symptom (in I'll investigate on that and try to get to the bottom, this is a bit suspicious |
Thanks for spotting and looking into it
…On Sat, Nov 19, 2022 at 4:10 PM Lorenzo Rubio ***@***.***> wrote:
thanks @dividor <https://github.com/dividor>
no, the self tests did not cover this
(always knew that our test coverage was not so great, but you don't expect
such a bug to come through!)
the test that is failing now is a new one I added in my last PR...
I don't see any other explanation that the bug being there all along...
but there is a very clear symptom (in all_tests_sumary.xlsx the column
rows_total is not equal to rows_failed + rows_passed, that's why I
noticed), which does not happen in the last execution I've got in my laptop
from Data-Observation-Toolkit.legacy
I'll investigate on that and try to get to the bottom, this is a bit
suspicious
—
Reply to this email directly, view it on GitHub
<#43 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACADNGXEFGFIXMMQPOFMERTWJE65FANCNFSM6AAAAAASFMD43U>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
finally got the problem, it is here the object that postgres gives back in either case is different in this PR I'll make it work in either of the 2 cases |
Thanks so much Lorenzo!
…On Tue, Nov 29, 2022 at 4:59 PM Lorenzo Rubio ***@***.***> wrote:
finally got the problem, it is here
<https://github.com/datakind/Data-Observation-Toolkit/blob/994ba4d0e9f05a018252eddeee6a48eaa6d89152/db/dot/3-demo_data.sql#L3>
in Muso we defined the uuid fields as text, and here as UUID
the object that postgres gives back in either case is different
in this PR I'll make it work in either of the 2 cases
—
Reply to this email directly, view it on GitHub
<#43 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACADNGTERT2SGEHT7XQ3R7TWKZ4DTANCNFSM6AAAAAASFMD43U>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
done @dividor! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, great stuff, thanks!
PII removed fields, merging for Medic prod release
@dividor when working on test type "possible_duplicate_forms" to add it to the self_tests, I've realised what I believe is a bug, but I'm very surprised we did not find it before
I'd appreciate your inputs in these questions:
uuid_list
in their dbt macros, and thus they are affected by the bug, thus if we are using any of these we would see an impactI would be happy if you prove that I'm making a silly mistake somewhere, but just don't see it!
thx