Add initial batch of unit tests for the React API by rgossiaux · Pull Request #305 · cardinalitypuzzles/cardboard

rgossiaux · 2021-01-06T10:30:22Z

These could be improved / made more complete but I think it's a start, and it's more testing than the old endpoints had anyway.

rgossiaux · 2021-01-06T11:03:10Z

I think the test failure is legitimate -- @asdfryan in #300 it looks like you didn't handle the case when puzzle.chat_room is None

erwa

I haven't looked closely at the tests you added, but just a thought: do we want to add JavaScript tests that directly test the React/Redux code? I think that would add additional robustness on top of testing the Python code.

rgossiaux · 2021-01-07T02:07:32Z

Eventually, yes. I don't want to spend the time on them before mystery hunt though because I think there are other more critical things to do, but I'm hoping to get to it after this mystery hunt. Added #307 to track.

erwa

Thanks for adding tests. Had a few comments.

erwa · 2021-01-07T13:57:10Z


 function getPuzzles(huntId) {
-  const puzzlesApiUrl = `/api/v1/hunt/${huntId}/puzzles`;
+  const puzzlesApiUrl = `/api/v1/hunts/${huntId}/puzzles`;


Can the common prefix "/api/v1/hunts" be defined as a constant in one place and then referenced elsewhere?

Yes. Actually I think the URL scheme is kind of bad, there's no need for the /hunts/:id/ prefix on most of the endpoints

I'm gonna update this in another PR after this because otherwise I think the merge conflicts will get a little messy. But I think I want to move:

/hunts/:id/puzzles/:id/tags => /puzzles/:id/tags
/hunts/:id/puzzles/:id/tags/:id => /puzzles/:id/tags/:id
/hunts/:id/puzzles/:id/answers => /puzzles/:id/answers
/hunts/:id/puzzles/:id/answers/:id => /puzzles/:id/answers/:id

Hopefully that's a little more readable. Then I'll change this api.js file to use a common /api/v1/ prefix

Ok, that's fine with me

erwa · 2021-01-07T13:59:40Z

+        # Malformatted URL (TODO)
+        # response = self.create_puzzle(
+        #     {"name": "test name", "url": "bad_url", "is_meta": False}
+        # )
+        # self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)


Can this be uncommented after #309?

Yeah, that was the idea / why I included this. If I merge this PR first (likely) I'll update the other one to uncomment this.

Oops, I linked to the wrong PR before, I meant #293

erwa · 2021-01-07T14:08:02Z

+if "unittest.util" in __import__("sys").modules:
+    # Show full diff in self.assertEqual.
+    __import__("sys").modules["unittest.util"]._MAX_LENGTH = 999999999


This seems a little strange to include here. I think you can just add -vv when running ./manage.py test to get more info including the full diff. I think it'd be better to increase the verbosity when running tests so it applies to all tests rather than making the change just for these api tests.

Oh nice, I didn't know about -v. But I think that doesn't prevent the test output from being truncated:

====================================================================== FAIL: test_create_puzzle (api.tests.ApiTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/src/smallboard/api/tests.py", line 49, in test_create_puzzle self.assertEqual( AssertionError: {'id'[137 chars]G', 'tags': [], 'guesses': [], 'metas': [], 'f[25 chars]alse} != {'id'[137 chars]G', 'guesses': [], 'metas': [], 'feeders': [],[13 chars]alse}

Since this runs during import I believe it applies to all tests as long as you're including api/tests.py but it wouldn't apply if you ran eg test answers, so I could move it somewhere else (maybe api/test_helpers.py?)

What if you use two vs as in literally -vv for extra verbosity?

usage: manage.py test [-h] [--noinput] [--failfast] [--testrunner TESTRUNNER] [-t TOP_LEVEL] [-p PATTERN] [--keepdb] [-r] [--debug-mode] [-d] [--parallel [N]] [--tag TAGS] [--exclude-tag EXCLUDE_TAGS] [--pdb] [-b] [-k TEST_NAME_PATTERNS] [--version] [-v {0,1,2,3}] [--settings SETTINGS] [--pythonpath PYTHONPATH] [--traceback] [--no-color] [--force-color] [test_label [test_label ...]] manage.py test: error: argument -v/--verbosity: invalid int value: 'v'

The previous comment was with the most permissive verbosity level (3).

I see... I am surprised at unittest's behavior.
I wonder if you can move this unittest configuration setup to the root directory and have it always be picked up?
If not, I would prefer not changing the MAX_LENGTH at all, since I feel this sort of configuration should be done globally (and put in a global place if so) rather than done in a specific test, even if it applies globally. But if you feel strongly for including it, I'm fine with keeping it, too.

I'll just delete it, probably simplest that way.

asdfryan · 2021-01-07T22:20:27Z

I think the test failure is legitimate -- @asdfryan in #300 it looks like you didn't handle the case when puzzle.chat_room is None

Oops -- will fix this.

Edit: NVM looks like you already fixed.

codecov · 2021-01-08T05:38:44Z

Codecov Report

Merging #305 (1611602) into master (5aa2024) will increase coverage by 5.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
+ Coverage   77.79%   83.23%   +5.43%     
==========================================
  Files         100      102       +2     
  Lines        1950     2099     +149     
==========================================
+ Hits         1517     1747     +230     
+ Misses        433      352      -81

Impacted Files	Coverage Δ
smallboard/settings.py	`89.04% <ø> (ø)`
answers/tests.py	`100.00% <100.00%> (ø)`
api/serializers.py	`95.12% <100.00%> (+26.82%)`	⬆️
api/test_helpers.py	`100.00% <100.00%> (ø)`
api/tests.py	`100.00% <100.00%> (ø)`
api/urls.py	`100.00% <100.00%> (ø)`
api/views.py	`89.61% <100.00%> (+63.96%)`	⬆️
puzzles/tests.py	`100.00% <100.00%> (ø)`
puzzles/views.py	`36.18% <0.00%> (-28.95%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aa2024...1611602. Read the comment docs.

asdfryan

Thanks! I commented some other edge cases worth testing. Feel free to merge this in first though. I can add those later.

asdfryan · 2021-01-08T05:52:14Z

+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data, {})
+        self.assertEqual(Puzzle.objects.count(), 0)


Another edge case worth testing: Trying to delete a puzzle that has already been deleted

asdfryan · 2021-01-08T05:54:35Z

+    def test_edit_puzzle(self):
+        self.create_puzzle({"name": "test name", "url": TEST_URL})
+        puzzle = Puzzle.objects.get()
+


Edge cases:

Editing more than 1 field at a time

Editing a non-editable field

New name matches a puzzle already in DB

asdfryan · 2021-01-08T05:55:52Z

+        self.assertEqual(puzzle.status, Puzzle.SOLVING)
+        self.assertEqual(len(puzzle.correct_answers()), 0)
+
+    def test_edit_answer(self):


Edge cases:

Editing an answer to another answer that already exists

Special characters / emojis

We should definitely test these but I think the answers model tests do/should take care of these ones

(though from the API side I think ideally we should still test that the API returns reasonable responses in these cases)

Renaming hunt => hunts and answer => answers for consistency and tweaking some small pieces of code.

These are still pretty imperfect/incomplete but it's a start

rgossiaux force-pushed the rgossiaux/api-tests branch from bf9b2ba to 0baecfa Compare January 6, 2021 11:00

erwa reviewed Jan 7, 2021

View reviewed changes

This was referenced Jan 7, 2021

Fix answer creation/deletion #309

Merged

Remove old UI code #310

Merged

erwa reviewed Jan 7, 2021

View reviewed changes

rgossiaux force-pushed the rgossiaux/api-tests branch from 10a7166 to 9f42b87 Compare January 8, 2021 05:35

rawxfish temporarily deployed to smallboard-rgossiaux-ap-d1gkrr January 8, 2021 05:38 Inactive

asdfryan approved these changes Jan 8, 2021

View reviewed changes

rgossiaux added 3 commits January 8, 2021 00:23

Make existing tests use API endpoints

daae09f

Make some small API adjustments

fcdf929

Renaming hunt => hunts and answer => answers for consistency and tweaking some small pieces of code.

Add initial batch of tests for the API

1611602

These are still pretty imperfect/incomplete but it's a start

rgossiaux force-pushed the rgossiaux/api-tests branch from 9f42b87 to 1611602 Compare January 8, 2021 08:23

rawxfish temporarily deployed to smallboard-rgossiaux-ap-d1gkrr January 8, 2021 08:24 Inactive

rgossiaux merged commit 0e83739 into master Jan 8, 2021

rgossiaux deleted the rgossiaux/api-tests branch January 8, 2021 08:26

rgossiaux mentioned this pull request Jan 9, 2021

Rewrite main hunt view in React #251

Closed

20 tasks

Conversation

rgossiaux commented Jan 6, 2021

Uh oh!

rgossiaux commented Jan 6, 2021

Uh oh!

erwa left a comment

Choose a reason for hiding this comment

Uh oh!

rgossiaux commented Jan 7, 2021

Uh oh!

erwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asdfryan commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

asdfryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asdfryan commented Jan 7, 2021 •

edited

Loading

codecov Bot commented Jan 8, 2021 •

edited

Loading