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.

Sign up for GitHub

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 #4829: Reduce code redundancy in backend testing #5838

Merged
merged 14 commits into from
Jan 6, 2019
Merged

Fix #4829: Reduce code redundancy in backend testing #5838

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3c32fb5
Replaced testapp.get with appropriate functions
Nov 8, 2018
846a5d8
Add test cases
Nov 23, 2018
982d983
Function get_response
Dec 2, 2018
ba2eafc
Function get_response_without_checking_for_errors
Dec 3, 2018
a04ffb5
Resolved conflicts
Dec 3, 2018
3b12b1e
Fixed Errors
Dec 3, 2018
014d687
Fixed lint checks
Dec 3, 2018
132f541
removed further redundancy
Dec 20, 2018
df8412f
Resolved conflicts
Dec 20, 2018
ab08985
Lint checks
Dec 20, 2018
1288bd5
Lint checks
Dec 20, 2018
acd0c1a
Merge develop
Jan 4, 2019
dddb91c
made params only dict type
Jan 6, 2019
08b1223
Minor changes
Jan 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Replaced testapp.get with appropriate functions
  • Loading branch information
Rishav Chakraborty committed Nov 8, 2018
commit 3c32fb5c524d3d6c83cb16c1f7f053d2ea498dff
26 changes: 14 additions & 12 deletions core/controllers/admin_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,28 @@ def setUp(self):
def test_admin_page_rights(self):
"""Test access rights to the admin page."""

response = self.testapp.get('/admin')
response = self.get_html('/admin', expect_errors=True,
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere, status codes beginning with 2 or 3 should not be considered errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's the case then should'nt the condition here change to assert both 2xx and 3xx codes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so. Thanks for catching that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Here and below, prefer breaking after '(' if contents of parens span multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

expected_status_int=302)
self.assertEqual(response.status_int, 302)
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need lines like this one, given that it's already checked above.

Ditto below and elsewhere (unless checking status_int is the only thing in the test).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


# Login as a non-admin.
self.login(self.EDITOR_EMAIL)
response = self.testapp.get('/admin', expect_errors=True)
response = self.get_html('/admin', expect_errors=True,
expected_status_int=401)
self.assertEqual(response.status_int, 401)
self.logout()

# Login as an admin.
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.testapp.get('/admin')
response = self.get_html('/admin')
self.assertEqual(response.status_int, 200)
self.logout()

def test_change_configuration_property(self):
"""Test that configuration properties can be changed."""

self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.testapp.get('/admin')
response = self.get_html('/admin')
csrf_token = self.get_csrf_token_from_response(response)

response_dict = self.get_json('/adminhandler')
Expand Down Expand Up @@ -87,11 +89,11 @@ def test_change_about_page_config_property(self):
"""Test that config property values are changed correctly."""
new_config_value = 'new_config_value'

response = self.testapp.get('/about')
response = self.get_html('/about')
self.assertNotIn(new_config_value, response.body)

self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.testapp.get('/admin')
response = self.get_html('/admin')
csrf_token = self.get_csrf_token_from_response(response)
self.post_json(
'/adminhandler', {
Expand All @@ -101,7 +103,7 @@ def test_change_about_page_config_property(self):
}
}, csrf_token=csrf_token)

response = self.testapp.get('/about')
response = self.get_html('/about')
self.assertIn(new_config_value, response.body)


Expand All @@ -111,7 +113,7 @@ class GenerateDummyExplorationsTest(test_utils.GenericTestBase):
def test_generate_count_greater_than_publish_count(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.testapp.get('/admin')
response = self.get_html('/admin')
csrf_token = self.get_csrf_token_from_response(response)
self.post_json(
'/adminhandler', {
Expand All @@ -127,7 +129,7 @@ def test_generate_count_greater_than_publish_count(self):
def test_generate_count_equal_to_publish_count(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.testapp.get('/admin')
response = self.get_html('/admin')
csrf_token = self.get_csrf_token_from_response(response)
self.post_json(
'/adminhandler', {
Expand All @@ -143,7 +145,7 @@ def test_generate_count_equal_to_publish_count(self):
def test_generate_count_less_than_publish_count(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.login(self.ADMIN_EMAIL, is_super_admin=True)
response = self.testapp.get('/admin')
response = self.get_html('/admin')
csrf_token = self.get_csrf_token_from_response(response)
generated_exps_response = self.post_json(
'/adminhandler', {
Expand Down Expand Up @@ -185,7 +187,7 @@ def test_view_and_update_role(self):
response_dict, {'user1': feconf.ROLE_ID_EXPLORATION_EDITOR})

# Check role correctly gets updated.
response = self.testapp.get(feconf.ADMIN_URL)
response = self.get_html(feconf.ADMIN_URL)
csrf_token = self.get_csrf_token_from_response(response)
response_dict = self.post_json(
feconf.ADMIN_ROLE_HANDLER_URL,
Expand Down Expand Up @@ -213,7 +215,7 @@ def test_invalid_username_in_view_and_update_role(self):
expected_status_int=400, expect_errors=True)

# Trying to update role of non-existent user.
response = self.testapp.get(feconf.ADMIN_URL)
response = self.get_html(feconf.ADMIN_URL)
csrf_token = self.get_csrf_token_from_response(response)
response = self.post_json(
feconf.ADMIN_ROLE_HANDLER_URL,
Expand Down
33 changes: 20 additions & 13 deletions core/controllers/base_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ def test_dev_indicator_appears_in_dev_and_not_in_production(self):
"""Test dev indicator appears in dev and not in production."""

with self.swap(constants, 'DEV_MODE', True):
response = self.testapp.get(feconf.LIBRARY_INDEX_URL)
response = self.get_html(feconf.LIBRARY_INDEX_URL)
self.assertIn('<div ng-if="DEV_MODE" class="oppia-dev-mode">',
response.body)

with self.swap(constants, 'DEV_MODE', False):
response = self.testapp.get(feconf.LIBRARY_INDEX_URL)
response = self.get_html(feconf.LIBRARY_INDEX_URL)
self.assertIn('<div ng-if="DEV_MODE" class="oppia-dev-mode">',
response.body)

Expand Down Expand Up @@ -106,10 +106,12 @@ def test_that_no_get_results_in_500_error(self):
def test_requests_for_invalid_paths(self):
"""Test that requests for invalid paths result in a 404 error."""

response = self.testapp.get('/library/extra', expect_errors=True)
response = self.get_html('/library/extra', expect_errors=True,
expected_status_int=404)
self.assertEqual(response.status_int, 404)

response = self.testapp.get('/library/data/extra', expect_errors=True)
response = self.get_html('/library/data/extra', expect_errors=True,
expected_status_int=404)
self.assertEqual(response.status_int, 404)

response = self.testapp.post(
Expand All @@ -123,7 +125,8 @@ def test_requests_for_invalid_paths(self):
def test_redirect_in_logged_out_states(self):
"""Test for a redirect in logged out state on '/'."""

response = self.testapp.get('/')
response = self.get_html('/', expect_errors=True,
expected_status_int=302)
self.assertEqual(response.status_int, 302)
self.assertIn('splash', response.headers['location'])

Expand All @@ -133,7 +136,8 @@ def test_root_redirect_rules_for_logged_in_learners(self):
# Since by default the homepage for all logged in users is the
# learner dashboard, going to '/' should redirect to the learner
# dashboard page.
response = self.testapp.get('/')
response = self.get_html('/', expect_errors=True,
expected_status_int=302)
self.assertEqual(response.status_int, 302)
self.assertIn('learner_dashboard', response.headers['location'])
self.logout()
Expand All @@ -151,7 +155,8 @@ def test_root_redirect_rules_for_users_with_no_user_contribution_model(
# Since by default the homepage for all logged in users is the
# learner dashboard, going to '/' should redirect to the learner
# dashboard page.
response = self.testapp.get('/')
response = self.get_html('/', expect_errors=True,
expected_status_int=302)
self.assertEqual(response.status_int, 302)
self.assertIn('learner_dashboard', response.headers['location'])
self.logout()
Expand All @@ -165,7 +170,8 @@ def test_root_redirect_rules_for_logged_in_creators(self):

# Since the default dashboard has been set as creator dashboard, going
# to '/' should redirect to the creator dashboard.
response = self.testapp.get('/')
response = self.get_html('/', expect_errors=True,
expected_status_int=302)
self.assertIn('creator_dashboard', response.headers['location'])

def test_root_redirect_rules_for_logged_in_editors(self):
Expand Down Expand Up @@ -195,7 +201,8 @@ def test_root_redirect_rules_for_logged_in_editors(self):

# Since user has edited one exploration created by another user,
# going to '/' should redirect to the dashboard page.
response = self.testapp.get('/')
response = self.get_html('/', expect_errors=True,
expected_status_int=302)
self.assertEqual(response.status_int, 302)
self.assertIn('dashboard', response.headers['location'])
self.logout()
Expand Down Expand Up @@ -279,7 +286,7 @@ def setUp(self):
def test_jinja_autoescaping(self):
form_url = '<[angular_tag]> x{{51 * 3}}y'
with self.swap(feconf, 'SITE_FEEDBACK_FORM_URL', form_url):
response = self.testapp.get('/fake')
response = self.get_html('/fake')
self.assertEqual(response.status_int, 200)

self.assertIn('&lt;[angular_tag]&gt;', response.body)
Expand All @@ -306,10 +313,10 @@ def test_logout_page(self):
# Logout with valid query arg. This test only validates that the login
# cookies have expired after hitting the logout url.
current_page = '/explore/0'
response = self.testapp.get(current_page)
response = self.get_html(current_page)
self.assertEqual(response.status_int, 200)
response = self.testapp.get(current_user_services.create_logout_url(
current_page))
response = self.get_html(current_user_services.create_logout_url(
current_page), expect_errors=True, expected_status_int=302)
expiry_date = response.headers['Set-Cookie'].rsplit('=', 1)

self.assertTrue(
Expand Down
22 changes: 12 additions & 10 deletions core/controllers/collection_editor_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,20 @@ def test_access_collection_editor_page(self):
'%s/%s?v=1' % (
feconf.COLLECTION_DATA_URL_PREFIX,
self.COLLECTION_ID))
self.assertEqual(response.status_int, 200)
self.assertEqual(response.status_code, 200)

# Check that non-editors cannot access the editor page. This is due
# to them not being whitelisted.
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (
feconf.COLLECTION_EDITOR_URL_PREFIX,
self.COLLECTION_ID))
self.COLLECTION_ID), expect_errors=True,
expected_status_int=302)
self.assertEqual(response.status_int, 302)

# Check that whitelisted users can access and edit in the editor page.
self.login(self.EDITOR_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (
feconf.COLLECTION_EDITOR_URL_PREFIX,
self.COLLECTION_ID))
Expand All @@ -104,10 +105,11 @@ def test_editable_collection_handler_get(self):

# Check that non-editors cannot access the editor data handler.
# This is due to them not being whitelisted.
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (
feconf.COLLECTION_EDITOR_DATA_URL_PREFIX,
self.COLLECTION_ID))
self.COLLECTION_ID), expect_errors=True,
expected_status_int=302)
self.assertEqual(response.status_int, 302)

# Check that whitelisted users can access the data
Expand Down Expand Up @@ -137,7 +139,7 @@ def test_editable_collection_handler_put_cannot_access(self):
self.login(self.VIEWER_EMAIL)

# Call get handler to return the csrf token.
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX,
self.COLLECTION_ID))
Expand Down Expand Up @@ -169,7 +171,7 @@ def test_editable_collection_handler_put_can_access(self):
self.login(self.EDITOR_EMAIL)

# Call get handler to return the csrf token.
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX,
self.COLLECTION_ID))
Expand Down Expand Up @@ -251,7 +253,7 @@ def test_publish_unpublish_collection(self):
collection_id, self.owner_id, exploration_id=exploration_id)
rights_manager.publish_exploration(self.owner, exploration_id)
collection = collection_services.get_collection_by_id(collection_id)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (
feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
Expand All @@ -264,7 +266,7 @@ def test_publish_unpublish_collection(self):

# Login as admin and unpublish the collection.
self.login(self.ADMIN_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
csrf_token = self.get_csrf_token_from_response(response)
response_dict = self.put_json(
Expand Down
20 changes: 10 additions & 10 deletions core/controllers/collection_viewer_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@ def setUp(self):
self.save_new_valid_collection(self.COLLECTION_ID, self.editor_id)

def test_unpublished_collections_are_invisible_to_logged_out_users(self):
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
expect_errors=True, expected_status_int=404)
self.assertEqual(response.status_int, 404)

def test_unpublished_collections_are_invisible_to_unconnected_users(self):
self.login(self.NEW_USER_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
expect_errors=True, expected_status_int=404)
self.assertEqual(response.status_int, 404)
self.logout()

Expand All @@ -60,15 +60,15 @@ def test_unpublished_collections_are_invisible_to_other_editors(self):
self.save_new_valid_collection('cid2', self.OTHER_EDITOR_EMAIL)

self.login(self.OTHER_EDITOR_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
expect_errors=True, expected_status_int=404)
self.assertEqual(response.status_int, 404)
self.logout()

def test_unpublished_collections_are_visible_to_their_editors(self):
self.login(self.EDITOR_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
self.assertEqual(response.status_int, 200)
self.logout()
Expand All @@ -77,15 +77,15 @@ def test_unpublished_collections_are_visible_to_admins(self):
self.signup(self.ADMIN_EMAIL, self.ADMIN_USERNAME)
self.set_admins([self.ADMIN_USERNAME])
self.login(self.ADMIN_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID))
self.assertEqual(response.status_int, 200)
self.logout()

def test_published_collections_are_visible_to_logged_out_users(self):
rights_manager.publish_collection(self.editor, self.COLLECTION_ID)

response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 200)
Expand All @@ -94,7 +94,7 @@ def test_published_collections_are_visible_to_logged_in_users(self):
rights_manager.publish_collection(self.editor, self.COLLECTION_ID)

self.login(self.NEW_USER_EMAIL)
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.COLLECTION_URL_PREFIX, self.COLLECTION_ID),
expect_errors=True)
self.assertEqual(response.status_int, 200)
Expand Down
4 changes: 2 additions & 2 deletions core/controllers/concept_card_viewer_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def test_get_concept_card(self):

def test_get_concept_card_fails_when_new_structures_not_enabled(self):
with self.swap(constants, 'ENABLE_NEW_STRUCTURES', False):
response = self.testapp.get(
response = self.get_html(
'%s/%s' % (feconf.CONCEPT_CARD_DATA_URL_PREFIX, self.skill_id),
expect_errors=True)
expect_errors=True, expected_status_int=404)
self.assertEqual(response.status_int, 404)
Loading