codeant-ai-for-open-source[bot] commented on code in PR #38360:
URL: https://github.com/apache/superset/pull/38360#discussion_r2879437707
##########
tests/unit_tests/extensions/test_types.py:
##########
@@ -236,7 +239,9 @@ def test_manifest_backend_no_files_field():
"publisher": "my-org",
"name": "my-extension",
"displayName": "My Extension",
- "backend": {"entryPoints": ["my_extension.entrypoint"]},
+ "backend": {
+ "entrypoint":
"superset_extensions.my_org.my_extension.entrypoint"
+ },
}
)
# ManifestBackend should not have a 'files' field
Review Comment:
**Suggestion:** The test that asserts `ManifestBackend` has no `files` field
never verifies that a backend object is actually created, so if `Manifest`
stops populating `backend`, the test will still pass because `hasattr(None,
"files")` is false, hiding potential regressions. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Missing backend object would not fail this test.
- ⚠️ Weak manifest schema validation for backend configuration.
```
</details>
```suggestion
assert manifest.backend is not None
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest
tests/unit_tests/extensions/test_types.py::test_manifest_backend_no_files_field`.
2. Inspect the test at `tests/unit_tests/extensions/test_types.py:234-248`;
it calls
`Manifest.model_validate` and then evaluates `hasattr(manifest.backend,
"files")` without
checking `manifest.backend` is not None.
3. In Python, confirm `hasattr(None, "files")` returns `False`, matching the
test's
expected assertion.
4. Therefore, if a regression causes `Manifest.model_validate` to stop
populating
`backend` (leaving it as None for this input), this test would still pass,
failing to
catch the schema regression around backend creation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_types.py
**Line:** 247:247
**Comment:**
*Logic Error: The test that asserts `ManifestBackend` has no `files`
field never verifies that a backend object is actually created, so if
`Manifest` stops populating `backend`, the test will still pass because
`hasattr(None, "files")` is false, hiding potential regressions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38360&comment_hash=76467c0b9bc8444ec32753c8ea00d88b9bb8837cdd770c846ac7e5c3af16eef0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38360&comment_hash=76467c0b9bc8444ec32753c8ea00d88b9bb8837cdd770c846ac7e5c3af16eef0&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/test_types.py:
##########
@@ -246,11 +251,14 @@ def test_manifest_backend_no_files_field():
def test_extension_config_backend_defaults():
"""Test ExtensionConfigBackend has correct defaults."""
backend = ExtensionConfigBackend.model_validate({})
- assert backend.entryPoints == []
assert backend.files == []
-def test_manifest_backend_defaults():
- """Test ManifestBackend has correct defaults."""
- backend = ManifestBackend.model_validate({})
- assert backend.entryPoints == []
+def test_manifest_backend_required_entrypoint():
+ """Test ManifestBackend requires entrypoint field."""
+ backend = ManifestBackend.model_validate(
+ {"entrypoint":
"superset_extensions.test_org.test_extension.entrypoint"}
+ )
+ assert (
+ backend.entrypoint ==
"superset_extensions.test_org.test_extension.entrypoint"
+ )
Review Comment:
**Suggestion:** The test claiming to verify that the backend entrypoint
field is required never actually checks the error path (missing entrypoint), so
it will still pass even if `ManifestBackend` stops enforcing this requirement,
weakening test coverage and allowing regressions to slip through. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ManifestBackend entrypoint requirement regression may go undetected.
- ⚠️ Weakens safety of extension backend loading tests.
```
</details>
```suggestion
with pytest.raises(ValidationError) as exc_info:
ManifestBackend.model_validate({})
assert "entrypoint" in str(exc_info.value)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest
tests/unit_tests/extensions/test_types.py::test_manifest_backend_required_entrypoint`.
2. Observe the test in `tests/unit_tests/extensions/test_types.py:257-264`
only calls
`ManifestBackend.model_validate` with a valid `entrypoint` and asserts
equality.
3. Note the docstring "Test ManifestBackend requires entrypoint field."
contradicts the
implementation, which never exercises the missing-entrypoint case.
4. Because the test never validates behavior when `entrypoint` is absent,
any future
change that makes `ManifestBackend.entrypoint` optional would not cause this
test to fail,
weakening regression protection around the required-entrypoint contract.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/test_types.py
**Line:** 259:264
**Comment:**
*Logic Error: The test claiming to verify that the backend entrypoint
field is required never actually checks the error path (missing entrypoint), so
it will still pass even if `ManifestBackend` stops enforcing this requirement,
weakening test coverage and allowing regressions to slip through.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38360&comment_hash=83d2964bb654aa197c6d6420c7d95b9b81b9af5359ab07487e0c3902d0a1d9c3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38360&comment_hash=83d2964bb654aa197c6d6420c7d95b9b81b9af5359ab07487e0c3902d0a1d9c3&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
