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

Update metadata tests #627

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Update metadata tests #627

merged 3 commits into from
Nov 5, 2021

Conversation

katxiao
Copy link
Contributor

@katxiao katxiao commented Nov 3, 2021

Resolves #624

@katxiao katxiao marked this pull request as ready for review November 4, 2021 01:26
@katxiao katxiao requested a review from a team as a code owner November 4, 2021 01:26
@katxiao katxiao requested review from pvk-developer and csala and removed request for a team November 4, 2021 01:26
tests/integration/metadata/test_table.py Outdated Show resolved Hide resolved
tests/integration/metadata/test_table.py Outdated Show resolved Hide resolved
}
}
metadata._fields_metadata = {
'foo': foo_metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest creating the other possible types of fields too, so we validate that the if logical checks are correct.
We should at least have:

  • A field of type id, which should be skipped
  • A field which is neither an id nor pii, which should also be skipped
  • A field which is not id but is pii, for which mappings should be created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added checks for other types of fields in the test__make_anonymization_mappings test. This test__make_anonymization_mappings_unique_faked_value_in_field just tests if there are repeats of a value in the id field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! My bad, I missed this. In any case, I like how you laid it out now!

tests/unit/metadata/test_table.py Outdated Show resolved Hide resolved
tests/unit/metadata/test_table.py Outdated Show resolved Hide resolved
Copy link
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

This looks good to me! I think it's ready to go.

}
}
metadata._fields_metadata = {
'foo': foo_metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! My bad, I missed this. In any case, I like how you laid it out now!

Copy link
Member

@pvk-developer pvk-developer left a comment

Choose a reason for hiding this comment

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

LGTM!

@katxiao katxiao merged commit 9a47807 into master Nov 5, 2021
@katxiao katxiao deleted the issue-624-update-metadata-tests branch November 5, 2021 17:53
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.

Add scaffolding for Metadata integration tests
3 participants