Skip to content

Tests now cover editing permissions#248

Open
songbird175 wants to merge 11 commits intodevfrom
bekins/auth-test
Open

Tests now cover editing permissions#248
songbird175 wants to merge 11 commits intodevfrom
bekins/auth-test

Conversation

@songbird175
Copy link
Contributor

@songbird175 songbird175 commented May 10, 2018

Closes #103.
There's one sub-test left to implement; I wasn't sure what invalid data would look like for delete.

@songbird175 songbird175 requested a review from osteele May 10, 2018 07:34
@@ -1,4 +1,4 @@
from unittest import skip
# from unittest import skip
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this line (and the following, blank, line). Generally don't leave commented-out code in a file, unless it's in a transitional state.

)
self.assertEqual(response.status_code, 200)
# TODO: test that the event has a new value
self.assertIn(b'new title', response.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like self.assertEqual(flask.json.loads(response.data)['title'], 'new title') will test the specific field. This will also be easier to read the intent of the test from.

content_type='application/json'
)
self.assertEqual(response.status_code, 200)
self.assertIn(b'new title', response.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above about testing for the actual title property .

)
self.assertEqual(response.status_code, 401)

with self.subTest("succeeds when required fields are present"):
Copy link
Contributor

Choose a reason for hiding this comment

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

This subtest description is misleading. It actually tests whether a user inside the local network can delete an event.

Copy link
Contributor

@osteele osteele left a comment

Choose a reason for hiding this comment

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

Looks good! Please address the comments, and then I'll merge it.

@songbird175 songbird175 dismissed osteele’s stale review May 10, 2018 14:24

Comments addressed.

@osteele
Copy link
Contributor

osteele commented May 10, 2018

There's one more comment, around with self.subTest("succeeds when required fields are present"); also a merge conflict.

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