[WEB-3707] pytest based test suite for apiserver#7010
[WEB-3707] pytest based test suite for apiserver#7010sriramveeraghanta merged 3 commits intopreviewfrom
Conversation
WalkthroughThis update restructures and enhances the testing framework for the Plane API server. It introduces new configuration files, comprehensive documentation, robust pytest fixtures, and a suite of unit, contract, and smoke tests. Many placeholder or legacy test files are removed, replaced by organized test modules and factories. Testing dependencies are updated and expanded, with scripts provided for streamlined test execution and coverage reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Runner (pytest)
participant Django as Django Test Server
participant Redis as Mocked Redis
participant DB as Test Database
participant Celery as Mocked Celery
Tester->>Django: Start test (e.g., POST /api/signin)
Django->>DB: Query user credentials
Django-->>Redis: (optional) Check/store magic link token
Django-->>Celery: (optional) Trigger background task (mocked)
Django-->>Tester: Return response (success or error)
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR establishes a pytest‑based test suite and restructures test documentation while updating the API authentication endpoint naming conventions. Key changes include the addition of external fixtures and general test utilities, the removal of several legacy API tests with accompanying documentation updates, and renaming of URL endpoints in the authentication module to consistently use a “space‑” prefix.
Reviewed Changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apiserver/plane/tests/conftest_external.py | Added fixtures to mock external services (Redis, Elasticsearch, MongoDB, Celery) for testing |
| apiserver/plane/tests/conftest.py | Introduced new test fixtures for API client, user, and token management |
| Multiple files under apiserver/plane/tests/api/ | Removed legacy test files; tests may be migrated or restructured under the new approach |
| apiserver/plane/tests/TESTING_GUIDE.md & README.md | Added documentation for the new testing strategy and directory structure |
| apiserver/plane/authentication/urls.py | Updated URL endpoint names for spaces endpoints to use a consistent “space-” prefix |
Files not reviewed (1)
- apiserver/.coveragerc: Language not supported
Comments suppressed due to low confidence (2)
apiserver/plane/authentication/urls.py:121
- Both the 'spaces/forgot-password/' and 'spaces/reset-password///' endpoints are assigned the same URL name 'space-forgot-password'. Consider renaming the reset-password endpoint to 'space-reset-password' to avoid reverse URL conflicts.
path("spaces/forgot-password/", ForgotPasswordSpaceEndpoint.as_view(), name="space-forgot-password"),
apiserver/plane/authentication/urls.py:130
- The 'change-password/' endpoint is assigned the URL name 'forgot-password', which is inconsistent and may lead to confusion. Consider updating the URL name to 'change-password'.
path("change-password/", ChangePasswordEndpoint.as_view(), name="forgot-password"),
aheckmann
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
apiserver/plane/authentication/urls.py (2)
112-112:⚠️ Potential issueInconsistent URL name for spaces/email-check endpoint
Unlike other space-related endpoints, the email-check endpoint for spaces doesn't have the "space-" prefix in its name attribute. This is inconsistent with the naming convention applied to other endpoints.
Apply this change to maintain consistency:
-path("spaces/email-check/", EmailCheckSpaceEndpoint.as_view(), name="email-check"), +path("spaces/email-check/", EmailCheckSpaceEndpoint.as_view(), name="space-email-check"),
118-118:⚠️ Potential issueDuplicate URL name "forgot-password" for different endpoints
Multiple endpoints (reset-password, change-password) are using the same URL name "forgot-password". This can cause reverse URL lookup issues in Django. Each endpoint should have a unique name.
Apply these changes to fix the naming conflicts:
- name="forgot-password", + name="reset-password",-path("change-password/", ChangePasswordEndpoint.as_view(), name="forgot-password"), +path("change-password/", ChangePasswordEndpoint.as_view(), name="change-password"),Also applies to: 130-130
🧹 Nitpick comments (24)
apiserver/plane/tests/factories.py (2)
44-55: Consider documenting the role valueThe role is set to 20 (Admin role) but the meaning of this value isn't immediately obvious from the code.
- role = 20 # Admin role by default + role = 20 # 20 = Admin role in the WorkspaceMember.RoleChoices enum
72-82: Same role documentation issue as in WorkspaceMemberFactorySimilar to the WorkspaceMemberFactory, consider documenting the role value more explicitly.
- role = 20 # Admin role by default + role = 20 # 20 = Admin role in the ProjectMember.RoleChoices enumapiserver/run_tests.py (2)
67-67: Potential duplication of verbose flagThe
-vflag is added here, but the pytest.ini already includes-vsin the addopts. This might be redundant or could lead to double verbose output.Consider modifying the verbose flag handling to account for the default settings in pytest.ini:
- if args.verbose: - cmd.append("-v") + # Only override verbosity if explicitly requested + if args.verbose: + # Replace default verbosity with explicit level + cmd.append("-vv") # More verbose than the default -vs
55-55: Consider using "and" for multiple markersThe current implementation uses "or" between markers, which runs tests matching any of the specified markers. In some cases, users might want to run tests matching all specified markers (e.g., tests that are both unit and smoke tests).
Consider adding an option to switch between "or" and "and" logic:
+ parser.add_argument( + "--require-all", + action="store_true", + help="Require all specified markers (AND logic instead of OR)" + ) # Add markers filter if markers: - cmd.extend(["-m", " or ".join(markers)]) + operator = " and " if args.require_all else " or " + cmd.extend(["-m", operator.join(markers)])apiserver/plane/tests/TESTING_GUIDE.md (3)
21-21: Fix trailing punctuation in Markdown headings.According to Markdown best practices, headings should not end with punctuation like colons.
-### Example Unit Test: +### Example Unit Test -### Example Contract Test: +### Example Contract Test -### Example Smoke Test: +### Example Smoke TestAlso applies to: 48-48, 74-74
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
21-21: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
106-113: Fix formatting in the fixtures list.The current line has a formatting issue that causes the numbered list to restart incorrectly.
-1. `api_client`: An unauthenticated DRF APIClient -2. `api_key_client`: API client with API key authentication (for external API tests) -3. `session_client`: API client with session authentication (for web app API tests) -4. `create_user`: Creates and returns a test user -5. `mock_redis`: Mocks Redis interactions -6. `mock_elasticsearch`: Mocks Elasticsearch interactions -7. `mock_celery`: Mocks Celery task execution +* `api_client`: An unauthenticated DRF APIClient +* `api_key_client`: API client with API key authentication (for external API tests) +* `session_client`: API client with session authentication (for web app API tests) +* `create_user`: Creates and returns a test user +* `mock_redis`: Mocks Redis interactions +* `mock_elasticsearch`: Mocks Elasticsearch interactions +* `mock_celery`: Mocks Celery task execution🧰 Tools
🪛 LanguageTool
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...everal useful fixtures: 1.api_client: An unauthenticated DRF APIClient 2. `ap...(UNLIKELY_OPENING_PUNCTUATION)
104-104: Add reference to fixture location.It would be helpful to mention where these fixtures are defined for easier discovery.
-Our test setup provides several useful fixtures: +Our test setup provides several useful fixtures (defined in `conftest.py` and `conftest_external.py`):apiserver/plane/tests/contract/app/test_workspace_app.py (2)
24-24: Consider breaking long line for better readability.The function signature exceeds the suggested line length limit of 88 characters.
- def test_create_workspace_valid_data(self, mock_workspace_seed, session_client, create_user): + def test_create_workspace_valid_data( + self, mock_workspace_seed, session_client, create_user + ):🧰 Tools
🪛 Ruff (0.11.9)
24-24: Line too long (97 > 88)
(E501)
77-79: Enhance assertion for duplicate slug error.The current test only checks that "slug" appears in the error response. A more specific assertion would ensure the error message is correctly communicated.
# Optionally check the error message to confirm it's related to the duplicate slug assert "slug" in response.data +# Verify the actual error message content +assert "already exists" in str(response.data["slug"][0]) # Or the expected error message🧰 Tools
🪛 Ruff (0.11.9)
78-78: Line too long (90 > 88)
(E501)
apiserver/plane/tests/unit/utils/test_uuid.py (1)
22-22: Clarify UUID version comment.The comment suggests the nil UUID is version 1, but it's actually version 0 (nil UUID). The test is correct since
is_valid_uuid()checks for version 4.- assert is_valid_uuid("00000000-0000-0000-0000-000000000000") is False # This is a valid UUID but version 1 + assert is_valid_uuid("00000000-0000-0000-0000-000000000000") is False # This is the nil UUID (version 0)🧰 Tools
🪛 Ruff (0.11.9)
22-22: Line too long (115 > 88)
(E501)
apiserver/plane/tests/README.md (2)
77-82: Consider consistent formatting for bullet pointsThe bullet points in the fixtures list should have consistent spacing after each bullet point.
- - `api_client`: Unauthenticated API client - - `create_user`: Creates a test user - - `api_token`: API token for the test user - - `api_key_client`: API client with API key authentication (for external API tests) - - `session_client`: API client with session authentication (for app API tests) - - `plane_server`: Live Django test server for HTTP-based smoke tests + - `api_client`: Unauthenticated API client + - `create_user`: Creates a test user + - `api_token`: API token for the test user + - `api_key_client`: API client with API key authentication (for external API tests) + - `session_client`: API client with session authentication (for app API tests) + - `plane_server`: Live Django test server for HTTP-based smoke tests🧰 Tools
🪛 LanguageTool
[uncategorized] ~77-~77: Loose punctuation mark.
Context: ...e available for testing: -api_client: Unauthenticated API client - `create_us...(UNLIKELY_OPENING_PUNCTUATION)
103-105: Consider consistent formatting for bullet pointsSimilar to the previous comment, ensure consistent spacing for these bullet points.
- - `conftest.py`: General fixtures for authentication, database access, etc. - - `conftest_external.py`: Fixtures for external services (Redis, Elasticsearch, Celery, MongoDB) - - `factories.py`: Test factories for easy model instance creation + - `conftest.py`: General fixtures for authentication, database access, etc. + - `conftest_external.py`: Fixtures for external services (Redis, Elasticsearch, Celery, MongoDB) + - `factories.py`: Test factories for easy model instance creation🧰 Tools
🪛 LanguageTool
[uncategorized] ~103-~103: Loose punctuation mark.
Context: ...ixtures are defined in: -conftest.py: General fixtures for authentication, da...(UNLIKELY_OPENING_PUNCTUATION)
apiserver/plane/tests/unit/models/test_workspace_model.py (2)
4-4: Remove unused importThe
Usermodel is imported but not directly used in this test file. It's used indirectly through thecreate_userfixture, but not explicitly in the test class.-from plane.db.models import Workspace, WorkspaceMember, User +from plane.db.models import Workspace, WorkspaceMember🧰 Tools
🪛 Ruff (0.11.9)
4-4:
plane.db.models.Userimported but unusedRemove unused import:
plane.db.models.User(F401)
43-44: Consider testing for default valuesWhile you're testing that the role was set correctly to 20 (Admin), consider also testing that default values from the model are set correctly when not explicitly provided. This would provide more comprehensive coverage of the model's behavior.
workspace_member = WorkspaceMember.objects.create( workspace=workspace, member=create_user, role=20 # Admin role ) + +# Create a workspace member with default role +default_workspace_member = WorkspaceMember.objects.create( + workspace=workspace, + member=create_user +) + +# Verify default values +assert default_workspace_member.role == 5 # Default role from model +assert default_workspace_member.is_active == Trueapiserver/plane/tests/smoke/test_auth_smoke.py (3)
37-38: Break up long assertion lineThis line exceeds the recommended line length (88 characters). Consider breaking it into multiple lines for better readability.
- assert "error" in data or "error_code" in data or "detail" in data or response.url.endswith("sign-in"), \ - "Error response should contain error details" + assert ("error" in data or + "error_code" in data or + "detail" in data or + response.url.endswith("sign-in")), \ + "Error response should contain error details"🧰 Tools
🪛 Ruff (0.11.9)
37-37: Line too long (125 > 88)
(E501)
76-77: Break up long assertion lineThis line also exceeds the recommended line length.
- assert "refresh_token" in data, "JWT auth should return both access and refresh tokens" + assert "refresh_token" in data, \ + "JWT auth should return both access and refresh tokens"🧰 Tools
🪛 Ruff (0.11.9)
76-76: Line too long (111 > 88)
(E501)
83-84: Break up long assertion lineAnother line that exceeds the recommended line length.
- assert not any(error_key in data for error_key in ["error", "error_code", "detail"]), \ - "Success response should not contain error keys" + error_keys = ["error", "error_code", "detail"] + assert not any(error_key in data for error_key in error_keys), \ + "Success response should not contain error keys"🧰 Tools
🪛 Ruff (0.11.9)
83-83: Line too long (111 > 88)
(E501)
apiserver/plane/tests/conftest.py (3)
2-6: Remove unused importsThere are several imported modules that aren't being used in this file.
import pytest -from django.conf import settings from rest_framework.test import APIClient from pytest_django.fixtures import django_db_setup -from unittest.mock import patch, MagicMock🧰 Tools
🪛 Ruff (0.11.9)
2-2:
django.conf.settingsimported but unusedRemove unused import:
django.conf.settings(F401)
5-5:
unittest.mock.patchimported but unusedRemove unused import
(F401)
5-5:
unittest.mock.MagicMockimported but unusedRemove unused import
(F401)
11-15: Simplify django_db_setup fixtureThis fixture redefines django_db_setup unnecessarily. Consider one of these approaches:
- Remove it completely if you're just using the default behavior
- Customize it with your specific database setup needs
- Rename it to avoid conflicts if you need both versions
If you're not customizing the default behavior, you can remove this:
-@pytest.fixture(scope="session") -def django_db_setup(django_db_setup): - """Set up the Django database for the test session""" - passIf you need to extend it, use a different name:
-@pytest.fixture(scope="session") -def django_db_setup(django_db_setup): - """Set up the Django database for the test session""" - pass +@pytest.fixture(scope="session") +def plane_db_setup(django_db_setup): + """Set up the Django database for the test session with Plane-specific configuration""" + # Add any custom database setup here + pass🧰 Tools
🪛 Ruff (0.11.9)
12-12: Redefinition of unused
django_db_setupfrom line 4(F811)
67-67: Break up long docstring lineThis docstring exceeds the recommended line length of 88 characters.
- """Return a session authenticated API client for app API testing, which is what plane.app uses""" + """ + Return a session authenticated API client for app API testing. + This is the authentication method used by plane.app. + """🧰 Tools
🪛 Ruff (0.11.9)
67-67: Line too long (101 > 88)
(E501)
apiserver/plane/tests/conftest_external.py (1)
1-3: Remove unused importThe
django.conf.settingsimport is not being used in this file.import pytest from unittest.mock import MagicMock, patch -from django.conf import settings🧰 Tools
🪛 Ruff (0.11.9)
3-3:
django.conf.settingsimported but unusedRemove unused import:
django.conf.settings(F401)
apiserver/plane/tests/contract/app/test_authentication.py (3)
1-14: Remove unused importThe
unittest.mock.MagicMockimport is not being used directly in this file.import json import uuid import pytest from django.urls import reverse from django.utils import timezone from rest_framework import status from django.test import Client from django.core.exceptions import ValidationError -from unittest.mock import patch, MagicMock +from unittest.mock import patch from plane.db.models import User from plane.settings.redis import redis_instance from plane.license.models import Instance🧰 Tools
🪛 Ruff (0.11.9)
9-9:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
165-172: Consider using more direct assertions for redirect checkingThe current implementation uses string concatenation and general string containment checks. A more direct approach might make the test clearer.
# Check for the specific authentication error in the URL -redirect_urls = [url for url, _ in response.redirect_chain] -redirect_contents = ' '.join(redirect_urls) # The actual error code for invalid password is AUTHENTICATION_FAILED_SIGN_IN -assert "AUTHENTICATION_FAILED_SIGN_IN" in redirect_contents +# Find if any redirect URL contains the error code +assert any("AUTHENTICATION_FAILED_SIGN_IN" in url for url, _ in response.redirect_chain)
383-383: Consider refactoring long assertion linesSeveral assertion lines exceed the recommended line length of 88 characters, including this one. Consider breaking them into multiple lines for better readability.
For example:
- assert "EXPIRED_MAGIC_CODE_SIGN_UP" in response.url or "INVALID_MAGIC_CODE_SIGN_UP" in response.url + assert ( + "EXPIRED_MAGIC_CODE_SIGN_UP" in response.url + or "INVALID_MAGIC_CODE_SIGN_UP" in response.url + )🧰 Tools
🪛 Ruff (0.11.9)
383-383: Line too long (107 > 88)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (32)
apiserver/.coveragerc(1 hunks)apiserver/plane/authentication/urls.py(3 hunks)apiserver/plane/tests/README.md(1 hunks)apiserver/plane/tests/TESTING_GUIDE.md(1 hunks)apiserver/plane/tests/__init__.py(1 hunks)apiserver/plane/tests/api/base.py(0 hunks)apiserver/plane/tests/api/test_asset.py(0 hunks)apiserver/plane/tests/api/test_auth_extended.py(0 hunks)apiserver/plane/tests/api/test_authentication.py(0 hunks)apiserver/plane/tests/api/test_cycle.py(0 hunks)apiserver/plane/tests/api/test_issue.py(0 hunks)apiserver/plane/tests/api/test_oauth.py(0 hunks)apiserver/plane/tests/api/test_people.py(0 hunks)apiserver/plane/tests/api/test_project.py(0 hunks)apiserver/plane/tests/api/test_shortcut.py(0 hunks)apiserver/plane/tests/api/test_state.py(0 hunks)apiserver/plane/tests/api/test_view.py(0 hunks)apiserver/plane/tests/api/test_workspace.py(0 hunks)apiserver/plane/tests/conftest.py(1 hunks)apiserver/plane/tests/conftest_external.py(1 hunks)apiserver/plane/tests/contract/app/__init__.py(1 hunks)apiserver/plane/tests/contract/app/test_authentication.py(1 hunks)apiserver/plane/tests/contract/app/test_workspace_app.py(1 hunks)apiserver/plane/tests/factories.py(1 hunks)apiserver/plane/tests/smoke/test_auth_smoke.py(1 hunks)apiserver/plane/tests/unit/models/test_workspace_model.py(1 hunks)apiserver/plane/tests/unit/serializers/test_workspace.py(1 hunks)apiserver/plane/tests/unit/utils/test_uuid.py(1 hunks)apiserver/pytest.ini(1 hunks)apiserver/requirements/test.txt(1 hunks)apiserver/run_tests.py(1 hunks)apiserver/run_tests.sh(1 hunks)
💤 Files with no reviewable changes (13)
- apiserver/plane/tests/api/test_cycle.py
- apiserver/plane/tests/api/test_state.py
- apiserver/plane/tests/api/test_view.py
- apiserver/plane/tests/api/test_oauth.py
- apiserver/plane/tests/api/test_asset.py
- apiserver/plane/tests/api/test_people.py
- apiserver/plane/tests/api/test_auth_extended.py
- apiserver/plane/tests/api/test_project.py
- apiserver/plane/tests/api/test_shortcut.py
- apiserver/plane/tests/api/test_issue.py
- apiserver/plane/tests/api/base.py
- apiserver/plane/tests/api/test_workspace.py
- apiserver/plane/tests/api/test_authentication.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
apiserver/plane/tests/unit/serializers/test_workspace.py (2)
apiserver/plane/app/serializers/workspace.py (1)
WorkspaceLiteSerializer(59-63)apiserver/plane/db/models/workspace.py (1)
Workspace(117-187)
apiserver/plane/tests/contract/app/test_workspace_app.py (2)
apiserver/plane/db/models/workspace.py (2)
Workspace(117-187)WorkspaceMember(207-239)apiserver/plane/tests/conftest.py (2)
session_client(66-69)create_user(35-44)
apiserver/plane/tests/unit/utils/test_uuid.py (1)
apiserver/plane/utils/uuid.py (2)
is_valid_uuid(6-12)convert_uuid_to_integer(15-22)
apiserver/plane/tests/smoke/test_auth_smoke.py (2)
apiserver/plane/tests/conftest.py (3)
plane_server(73-78)create_user(35-44)user_data(24-31)apiserver/plane/settings/storage.py (1)
url(15-16)
apiserver/plane/tests/unit/models/test_workspace_model.py (2)
apiserver/plane/db/models/workspace.py (2)
Workspace(117-187)WorkspaceMember(207-239)apiserver/plane/tests/conftest.py (1)
create_user(35-44)
apiserver/plane/tests/contract/app/test_authentication.py (1)
apiserver/plane/tests/conftest.py (2)
api_client(18-20)user_data(24-31)
🪛 LanguageTool
apiserver/plane/tests/TESTING_GUIDE.md
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...everal useful fixtures: 1. api_client: An unauthenticated DRF APIClient 2. `ap...
(UNLIKELY_OPENING_PUNCTUATION)
apiserver/plane/tests/README.md
[uncategorized] ~77-~77: Loose punctuation mark.
Context: ...e available for testing: - api_client: Unauthenticated API client - `create_us...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~92-~92: Possible typo: you repeated a word.
Context: ...t - For smoke tests with real HTTP, useplane_server` 3. Use the correct URL namespace when reverse-...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~103-~103: Loose punctuation mark.
Context: ...ixtures are defined in: - conftest.py: General fixtures for authentication, da...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
apiserver/plane/tests/TESTING_GUIDE.md
21-21: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
48-48: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
74-74: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 Ruff (0.11.9)
apiserver/plane/tests/conftest_external.py
3-3: django.conf.settings imported but unused
Remove unused import: django.conf.settings
(F401)
56-56: Line too long (90 > 88)
(E501)
apiserver/plane/tests/contract/app/test_workspace_app.py
24-24: Line too long (97 > 88)
(E501)
27-27: Line too long (94 > 88)
(E501)
78-78: Line too long (90 > 88)
(E501)
apiserver/plane/tests/unit/utils/test_uuid.py
22-22: Line too long (115 > 88)
(E501)
49-49: Line too long (91 > 88)
(E501)
apiserver/plane/tests/smoke/test_auth_smoke.py
28-28: Line too long (91 > 88)
(E501)
32-32: Line too long (91 > 88)
(E501)
37-37: Line too long (125 > 88)
(E501)
60-60: Line too long (94 > 88)
(E501)
76-76: Line too long (111 > 88)
(E501)
79-79: Line too long (89 > 88)
(E501)
83-83: Line too long (111 > 88)
(E501)
apiserver/plane/tests/unit/models/test_workspace_model.py
4-4: plane.db.models.User imported but unused
Remove unused import: plane.db.models.User
(F401)
apiserver/plane/tests/conftest.py
2-2: django.conf.settings imported but unused
Remove unused import: django.conf.settings
(F401)
5-5: unittest.mock.patch imported but unused
Remove unused import
(F401)
5-5: unittest.mock.MagicMock imported but unused
Remove unused import
(F401)
12-12: Redefinition of unused django_db_setup from line 4
(F811)
67-67: Line too long (101 > 88)
(E501)
apiserver/plane/tests/contract/app/test_authentication.py
9-9: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
19-19: Line too long (96 > 88)
(E501)
39-39: Line too long (117 > 88)
(E501)
82-82: Line too long (91 > 88)
(E501)
100-100: Line too long (97 > 88)
(E501)
213-213: Line too long (103 > 88)
(E501)
240-240: Line too long (89 > 88)
(E501)
257-257: Line too long (107 > 88)
(E501)
274-274: Line too long (110 > 88)
(E501)
283-283: Line too long (95 > 88)
(E501)
305-305: Line too long (120 > 88)
(E501)
314-314: Line too long (95 > 88)
(E501)
383-383: Line too long (107 > 88)
(E501)
387-387: Line too long (98 > 88)
(E501)
398-398: Line too long (95 > 88)
(E501)
423-423: Line too long (108 > 88)
(E501)
434-434: Line too long (95 > 88)
(E501)
453-453: Line too long (103 > 88)
(E501)
🔇 Additional comments (44)
apiserver/plane/tests/factories.py (4)
1-11: Well-structured imports and model selectionThe factory implementation includes all necessary imports and only the specific models required for the current factories. This demonstrates good organization and avoids polluting the namespace with unnecessary imports.
14-28: Good implementation of UserFactory with proper password handlingThe UserFactory correctly implements the password hashing using
PostGenerationMethodCall, which is the recommended approach for Django user factories. The use of sequences for emails and names ensures uniqueness in tests.
30-42: Proper workspace factory implementation with owner relationshipThe WorkspaceFactory correctly establishes the relationship with User through SubFactory and handles timestamps appropriately. The use of
django_get_or_createwithslugwill help prevent duplicate test objects.
57-70: Intelligent use of SelfAttribute for created_by and updated_byGood job using
SelfAttributeto reference the workspace owner forcreated_byandupdated_by. This ensures consistency in the relationship between workspace and project creators.apiserver/plane/tests/contract/app/__init__.py (1)
1-1: Empty__init__.pycorrectly marks the directory as a Python packageThe empty
__init__.pyfile correctly marks the directory as a Python package, enabling proper module imports in the test suite. This follows Python package structure best practices.apiserver/plane/tests/__init__.py (1)
1-2: Good removal of wildcard importReplacing the wildcard import with a simple comment is a good practice. Wildcard imports can lead to namespace pollution and make it harder to track dependencies. This change aligns with the restructuring of the test suite to a more modular approach.
apiserver/run_tests.sh (1)
1-4: Simple and effective test wrapper scriptThe wrapper script is concise and effective. The use of
execis appropriate as it replaces the current process rather than spawning a subshell, which preserves any exit codes from the test runner. The forwarding of all arguments ("$@") ensures flexibility in how tests can be run.apiserver/.coveragerc (2)
1-13: Well-configured coverage sources and exclusionsThe [run] section properly configures the coverage tool to focus on application code by targeting the
planedirectory and excluding test-related files, migrations, configuration files, and other non-application code. This approach will result in more accurate and meaningful coverage metrics.
14-23: Appropriate coverage exclusion patternsThe exclude_lines configuration follows best practices by omitting non-functional code patterns from coverage requirements. This prevents developers from writing unnecessary tests for debug statements, placeholder methods, import guards, and other code that doesn't need explicit test coverage.
apiserver/requirements/test.txt (1)
2-12: Comprehensive test dependenciesExcellent selection of testing libraries that enable robust testing practices:
- Core pytest with django integration
- Coverage and parallel test execution support
- Mocking capabilities with pytest-mock
- Test data generation with factory-boy
- Time control with freezegun
- HTTP client libraries for API testing
The versions chosen are recent and stable, providing a solid foundation for the test suite.
apiserver/plane/authentication/urls.py (2)
45-46: Consistent naming convention for space-related endpointsThe URL route names have been standardized by adding the "space-" prefix to all space-related authentication endpoints. This improves code clarity and makes the distinction between app-level and space-level routes more explicit.
Also applies to: 49-49, 59-59, 64-64, 69-69, 77-77, 82-82, 90-90, 95-95, 103-103, 108-108, 123-123, 128-128
80-80: Fixed path prefix for Google callbackThe URL path now correctly includes the "spaces/" prefix, matching the pattern used by other OAuth callback endpoints. This fixes an inconsistency in the URL structure.
apiserver/pytest.ini (3)
1-6: Well-configured pytest settings for DjangoThe configuration correctly sets up the Django test environment by specifying the test settings module and defining test discovery patterns for files, classes, and functions following pytest conventions.
7-11: Well-defined test markersThe markers are clearly defined and categorized into different test types (unit, contract, smoke, slow), which allows for selective test execution and better organization of the test suite.
13-17: Optimized test execution optionsThe addopts configuration improves the testing workflow by:
- Enforcing strict marker usage for better consistency
- Reusing the test database to speed up test execution
- Skipping migrations for faster startup
- Enabling verbose output and print statement capture for better debugging
These settings will significantly improve the developer experience when running tests.
apiserver/run_tests.py (3)
7-39: Well-structured command-line interfaceThe argument parser is clearly organized with logical options for running different types of tests, controlling coverage reporting, enabling parallel execution, and toggling verbose output. The help messages are descriptive and user-friendly.
41-70: Flexible test command constructionThe command building logic correctly assembles pytest commands based on user-selected options, providing a flexible way to run specific test types with appropriate configuration. The marker selection logic is particularly useful for targeting specific test categories.
78-86: Strong coverage enforcementThe script enforces a 90% coverage threshold when coverage reporting is enabled, which promotes high test quality. The clear error message and proper exit code handling ensure that coverage issues are visible in CI pipelines.
apiserver/plane/tests/TESTING_GUIDE.md (3)
3-11: Well-structured introduction to the testing approach.The guide provides a clear structure for the testing strategy with distinct test categories.
15-20: Good organization of unit test directories.The clear directory structure will help developers locate and organize their tests appropriately.
145-151: Excellent best practices for test writing.The list of best practices provides valuable guidance that will help maintain test quality.
apiserver/plane/tests/unit/serializers/test_workspace.py (2)
12-41: Good test for serializer field validation.The test effectively verifies that the serializer correctly includes and processes the expected fields from a Workspace instance.
42-71: Effective test for read-only field behavior.This test correctly verifies that the serializer treats its fields as read-only by attempting to update them and confirming the changes are not persisted.
apiserver/plane/tests/contract/app/test_workspace_app.py (3)
13-21: Good test for empty data validation.The test properly verifies that sending an empty request to create a workspace results in a 400 Bad Request response.
22-58: Well-structured test for valid workspace creation.This test effectively verifies multiple aspects of workspace creation:
- Correct HTTP status code
- Database record creation
- Proper relationship setup between workspace and user
- Background task triggering
Excellent coverage of both API behavior and side effects.
🧰 Tools
🪛 Ruff (0.11.9)
24-24: Line too long (97 > 88)
(E501)
27-27: Line too long (94 > 88)
(E501)
59-79: Good test for duplicate workspace validation.The test correctly verifies that creating a duplicate workspace with the same slug is prevented.
🧰 Tools
🪛 Ruff (0.11.9)
78-78: Line too long (90 > 88)
(E501)
apiserver/plane/tests/unit/utils/test_uuid.py (4)
42-49: Excellent test for string input handling.This test is valuable as it verifies that the function behaves consistently regardless of whether it receives a UUID object or string, which promotes more flexible API usage.
🧰 Tools
🪛 Ruff (0.11.9)
49-49: Line too long (91 > 88)
(E501)
10-15: Good test for valid UUID validation.The test verifies that a dynamically generated UUID is correctly validated as a valid UUID.
16-23: Comprehensive testing of invalid UUID cases.The test correctly verifies multiple invalid UUID scenarios, including checking that non-version-4 UUIDs are rejected. This helps ensure the utility function's robustness.
🧰 Tools
🪛 Ruff (0.11.9)
22-22: Line too long (115 > 88)
(E501)
24-41: Thorough testing of UUID conversion functionality.The test verifies important properties of the conversion function:
- Returns an integer
- Produces consistent results for the same input
- Produces different results for different inputs
This validates both correctness and stability of the conversion algorithm.
apiserver/plane/tests/README.md (1)
1-143: Great comprehensive testing documentation!This README provides an excellent overview of the testing strategy, organization, and best practices. The categorization of tests into unit, contract, and smoke tests with clear explanations will help developers understand the testing approach.
I particularly appreciate the detailed instructions for running tests, the explanation of test fixtures, and the clear guidelines for writing new tests. The distinction between API vs App endpoints is well-documented.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~77-~77: Loose punctuation mark.
Context: ...e available for testing: -api_client: Unauthenticated API client - `create_us...(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~92-~92: Possible typo: you repeated a word.
Context: ...t- For smoke tests with real HTTP, useplane_server` 3. Use the correct URL namespace when reverse-...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~103-~103: Loose punctuation mark.
Context: ...ixtures are defined in: -conftest.py: General fixtures for authentication, da...(UNLIKELY_OPENING_PUNCTUATION)
apiserver/plane/tests/unit/models/test_workspace_model.py (1)
7-50: Well-structured model testsThese unit tests effectively verify the creation and basic properties of Workspace and WorkspaceMember models. The tests are appropriately marked with the required decorators and follow pytest best practices.
apiserver/plane/tests/smoke/test_auth_smoke.py (2)
6-88: Robust authentication smoke testsThe smoke tests for authentication are well-designed and resilient to implementation changes. They effectively test both failure and success cases, handle different response types (redirects, JSON), and make appropriate assertions about the expected behavior.
🧰 Tools
🪛 Ruff (0.11.9)
28-28: Line too long (91 > 88)
(E501)
32-32: Line too long (91 > 88)
(E501)
37-37: Line too long (125 > 88)
(E501)
60-60: Line too long (94 > 88)
(E501)
76-76: Line too long (111 > 88)
(E501)
79-79: Line too long (89 > 88)
(E501)
83-83: Line too long (111 > 88)
(E501)
90-100: Effective health check smoke testThe health check test is simple but effective in verifying the basic availability of the application.
apiserver/plane/tests/conftest.py (1)
17-78: Well-designed test fixturesThe test fixtures are well-organized, properly documented, and provide a good foundation for the testing framework. They cover the different types of authentication needed for the various API endpoints and provide utility functions for creating test data.
🧰 Tools
🪛 Ruff (0.11.9)
67-67: Line too long (101 > 88)
(E501)
apiserver/plane/tests/conftest_external.py (4)
6-25: Well-implemented Redis mock fixtureThis fixture correctly mocks the Redis client by configuring common operations and patching the right import path. The implementation follows pytest fixture best practices.
28-48: Good Elasticsearch mock implementationThe fixture properly configures mock responses for common Elasticsearch operations like search, index, update, and delete. The patch target is correct and the mock behavior is appropriate for tests.
51-104: Comprehensive MongoDB mock setupThis fixture does an excellent job of creating the full MongoDB client chain (client -> database -> collection) and configuring appropriate return values for all common operations.
🧰 Tools
🪛 Ruff (0.11.9)
56-56: Line too long (90 > 88)
(E501)
107-117: Effective Celery task mockingThe mock_celery fixture correctly patches the Task.delay method to prevent actual task execution during tests. The mock is configured with a reasonable return value.
apiserver/plane/tests/contract/app/test_authentication.py (5)
16-34: LGTM: Well-structured Instance fixtureThe setup_instance fixture correctly handles both creation and update scenarios, setting all required fields for the Instance model. This provides a good foundation for the authentication tests.
🧰 Tools
🪛 Ruff (0.11.9)
19-19: Line too long (96 > 88)
(E501)
43-113: Comprehensive magic link generation testsThe TestMagicLinkGenerate class provides thorough coverage of the magic link generation endpoint, including:
- Empty data validation
- Email format validation
- Successful generation
- Rate limiting (max attempts)
The tests properly mock external dependencies and validate both successful and error responses.
🧰 Tools
🪛 Ruff (0.11.9)
82-82: Line too long (91 > 88)
(E501)
100-100: Line too long (97 > 88)
(E501)
115-216: Thorough sign-in endpoint testsThe TestSignInEndpoint class provides excellent coverage of all sign-in scenarios:
- Empty data validation
- Email format validation
- Non-existent user handling
- Incorrect password handling
- Successful login
- Next path redirection
The tests properly check for redirects, error codes in URLs, and session authentication state.
🧰 Tools
🪛 Ruff (0.11.9)
213-213: Line too long (103 > 88)
(E501)
218-337: Well-structured magic sign-in testsThe TestMagicSignIn class thoroughly tests the magic link sign-in flow, including:
- Empty data validation
- Expired/invalid link handling
- Non-existent user handling
- Successful sign-in
- Next path redirection
The tests appropriately set up Redis state and verify both redirects and authentication status.
🧰 Tools
🪛 Ruff (0.11.9)
240-240: Line too long (89 > 88)
(E501)
257-257: Line too long (107 > 88)
(E501)
274-274: Line too long (110 > 88)
(E501)
283-283: Line too long (95 > 88)
(E501)
305-305: Line too long (120 > 88)
(E501)
314-314: Line too long (95 > 88)
(E501)
339-459: Complete magic sign-up test coverageThe TestMagicSignUp class provides thorough testing of the magic link sign-up flow, including:
- Empty data validation
- Existing user handling
- Expired/invalid link handling
- Successful sign-up
- Next path redirection
The tests verify user creation and authentication state appropriately.
🧰 Tools
🪛 Ruff (0.11.9)
383-383: Line too long (107 > 88)
(E501)
387-387: Line too long (98 > 88)
(E501)
398-398: Line too long (95 > 88)
(E501)
423-423: Line too long (108 > 88)
(E501)
434-434: Line too long (95 > 88)
(E501)
453-453: Line too long (103 > 88)
(E501)
|
🥳 |
* pytest bases tests for apiserver * Trimmed spaces * Updated .gitignore for pytest local files
Description
Define and implement a unified, layered testing strategy for Plane’s Django REST Framework (DRF)-based backend API using pytest and the pytest-django plugin. This approach will ensure fast feedback during development, high-confidence releases, and maintainable test code.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
The test cases can be run using the helper commands provided in this PR which internally calls the correct pytest command with the required parameters.
References
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores