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

add self test for test type possible_duplicate_forms #43

Merged
merged 12 commits into from
Dec 3, 2022

Conversation

lrnzcig
Copy link
Collaborator

@lrnzcig lrnzcig commented Nov 19, 2022

@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:

  • what did you have in mind with this comment, if you remember?
  • is it by any chance possible that we are not using any of these test types in Muso: "possible_duplicate_forms", "associated_columns_not_null", "relationships", "not_negative_string_column"? -these are the ones using 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 impact

I would be happy if you prove that I'm making a silly mistake somewhere, but just don't see it!
thx

@lrnzcig
Copy link
Collaborator Author

lrnzcig commented Nov 19, 2022

note that the tests are failing since this affects the test totals (number of rows that have failed) and the failed rows themselves

@dividor
Copy link
Contributor

dividor commented Nov 19, 2022

Hi @lrnzcig,

  • "what did you have in mind with this comment, if you remember?"

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.

  • "is it by any chance possible that we are not using any of these test types in Muso: "possible_duplicate_forms", "associated_columns_not_null", "relationships", "not_negative_string_column"? -these are the ones using 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 impact"

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!

@lrnzcig
Copy link
Collaborator Author

lrnzcig commented Nov 19, 2022

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 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

@dividor
Copy link
Contributor

dividor commented Nov 19, 2022 via email

@lrnzcig
Copy link
Collaborator Author

lrnzcig commented Nov 29, 2022

finally got the problem, it is here
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

@dividor
Copy link
Contributor

dividor commented Nov 30, 2022 via email

@lrnzcig lrnzcig marked this pull request as ready for review December 1, 2022 20:53
@lrnzcig
Copy link
Collaborator Author

lrnzcig commented Dec 1, 2022

done @dividor!

@lrnzcig lrnzcig changed the base branch from test-all-test-types to main December 1, 2022 20:54
@lrnzcig lrnzcig changed the title bugfix: transform uuid_list properly from postgres into a list add self test for test type possible_duplicate_forms Dec 1, 2022
Copy link
Contributor

@dividor dividor left a 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!

@dividor dividor merged commit 97cb269 into main Dec 3, 2022
dividor added a commit that referenced this pull request Dec 4, 2022
PII removed fields, merging for Medic prod release
@lrnzcig lrnzcig deleted the test-possible-duplicate-forms branch December 10, 2022 11:48
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.

2 participants