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 test when using locale middleware #300

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Fix test when using locale middleware #300

merged 2 commits into from
Apr 25, 2017

Conversation

willbarton
Copy link
Member

When running the college-costs tests with https://github.com/cfpb/cfgov-refresh/, the django locale middleware is loaded. That middleware chokes trying to modify the headers of the original MagicMock it got instead of a Response object. This PR removes the mocking of render_to_response and mocks the feedback object creation instead.

Part of GHE/CFGOV/platform/issues/798

Changes

Testing

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cc151cf on holistic-tests into 0a2dda8 on master.

reverse('disclosures:pfc-feedback'),
data=self.feedback_post_data)
self.assertTrue(mock_render.call_count == 1)
self.assertEqual(response.status_code, 200)
self.assertTrue(mock_feedback.call_count == 1)
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine. One minor suggestion: this test seems to be testing that the Feedback constructor is called when this view is POSTed to. This seems a bit specific, for example the view might create the feedback object in some other way (e.g. Feedback.objects.create(...)). It might be more clarifying to rename this test slightly more specifically (like test_feedback_post_creates_feedback) and then do something like self.assertFalse(Feedback.objects.exists()) before and then self.assertTrue(Feedback.objects.exists()) after.

Copy link
Member Author

Choose a reason for hiding this comment

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

@chosak good point. I'm not entire sure what the original intent was here. @higs4281?

Copy link
Member

@higs4281 higs4281 Apr 25, 2017

Choose a reason for hiding this comment

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

@willbarton I join you in not being entirely sure of the author's intent, but test_feedback_post_creates_feedback seems clearer in this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

@higs4281
Copy link
Member

Wouldn't #299 make a handsome pairing for a release?
??

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8c27c4e on holistic-tests into 0a2dda8 on master.

@willbarton willbarton merged commit 0af0fa5 into master Apr 25, 2017
@willbarton willbarton deleted the holistic-tests branch April 25, 2017 20:39
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.

None yet

5 participants