Skip to content

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

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.

3 participants