Skip to content

Remove remaining wip pieces in tests #917

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

Merged
merged 4 commits into from
Sep 1, 2022

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented May 26, 2022

A few tests in #903 still had WIP references. Removed those.

@dshemetov dshemetov requested a review from melange396 June 1, 2022 23:49
Base automatically changed from v4-schema-revisions-release-prep-prep to v4-schema-revisions-release-prep June 15, 2022 20:14
Copy link
Collaborator

@melange396 melange396 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 great, but im confused by the one line change that i pointed out...

@@ -87,30 +83,28 @@ def load_csv_impl(path, *args):
mock_logger,
csv_importer_impl=mock_csv_importer)

self.assertEqual(modified_row_count, 4)
self.assertEqual(modified_row_count, 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i know this isnt exactly recent in your memory anymore, but why did this change to 1 instead of to 3??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question, let me look around. since tests pass, there must've been some change to this test in v4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing this out! i made a fix, so it's 3 now. here's an explanation:

  • we have a database mock object, which overwrites insert_or_update_bulk
  • we didn't even count how many rows this function was passed, we just updated the mock return_value argument
  • so i rewrote this so insert_or_update_bulk = MagicMock(wraps=iter_len) where iter_len is just a function that counts the number of items in the iterable passed to insert_or_update_bulk. so now we correctly get modified_row_count == 3

@krivard krivard requested a review from melange396 September 1, 2022 13:37
Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

👍

@melange396 melange396 merged commit 2e49339 into v4-schema-revisions-release-prep Sep 1, 2022
@melange396 melange396 deleted the ds/remove-wip-tests branch September 1, 2022 16:25
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