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

Fix assert_matching_tables #759

Merged
merged 8 commits into from
Oct 20, 2022
Merged

Conversation

crayolakat
Copy link
Collaborator

@crayolakat crayolakat commented Oct 18, 2022

In utils.py, when comparing assert list(r1) == list(r2), if r1 and r2 are typedict, this only compares if the headers of r1 and r2 are equal, when it should be comparing if the entire contents of both dicts are equal.

This is why the test_google_admin.py test was passing even though it had a typo and should have failed. This typo is also fixed in this PR

The csv_renewed and distribute_task functions return a table with the ints converted to strs
The function definition of match_column states that if there are 2 columns with the same normalized name, the latter should be removed by default. The normalize_fn function must run over the reversed list of self.columns in order for the first matching column to be kept. In it's previous state, the last matching column was kept
To match order in airtable_responses.py
Current test is returning the Lyndon Johnson lead twice
Fix typo in test. Also change import unittest to import unittest.mock to avoid `AttributeError: module 'unittest' has no attribute 'mock'` when running test locally
@crayolakat
Copy link
Collaborator Author

crayolakat commented Oct 20, 2022

After this change, a few CircleCI tests failed. This is because those tests either had typos or flaws which weren't being caught in the assert_matching_tables function since the function previously only checked if the headers of the dict tables matched.

I went ahead and fixed those tests.

Copy link
Collaborator

@shaunagm shaunagm left a comment

Choose a reason for hiding this comment

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

We already talked about this in the Parsons Party, but (a) this looks good to me and (b) thank you so much for catching and fixing this!

@shaunagm shaunagm merged commit 17df1da into move-coop:main Oct 20, 2022
@crayolakat crayolakat deleted the kathy_fix_tests branch October 20, 2022 19:51
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