Refactor tests#32
Merged
chris-adam merged 1 commit intomainfrom May 5, 2026
Merged
Conversation
📝 WalkthroughWalkthroughReplace per-test manual setup with a shared BaseEsignTest and seeded fixtures; refactor many tests to use the base, add/reshape tests for external session creation, feedback POST, VAT validation, and removal flows; adjust create_external_session to use per-file ChangesTest base + test refactor + utils
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Coverage Report for CI Build 25375500821Coverage increased (+7.9%) to 84.566%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
chris-adam
force-pushed
the
PARAF-360/esign_tests
branch
5 times, most recently
from
8730123 to
27fc56f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/imio/esign/tests/test_utils.py (1)
474-479: Lock in the multipartfiles=shape as well.These assertions only cover the JSON body. The production change also switched
requests.post(..., files=...)to a list of repeated"files"tuples so duplicate filenames survive, and that behavior is still untested here. Add one assertion onmock_post.call_args[1]["files"]so this regression path stays covered.Example assertion to add next to the first payload check
+ self.assertEqual( + [(field, filename) for field, (filename, _content) in mock_post.call_args[1]["files"]], + [("files", "annex1.pdf"), ("files", "annex2.pdf")], + )Also applies to: 512-517, 554-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/test_utils.py` around lines 474 - 479, The test currently only asserts the JSON payload from create_external_session; add an assertion that mock_post.call_args[1]["files"] is the expected multipart shape (a list of ("files", (filename, content, content_type)) tuples) to confirm duplicate filenames survive and the code sends a list of repeated "files" tuples; update the assertion next to the existing payload check in test_utils.py (around the create_external_session call) to validate mock_post.call_args[1]["files"] is a list and contains the expected repeated filename entries (and apply the same additional files assertion at the other two test locations referenced in the comment).src/imio/esign/tests/base.py (1)
3-5: Reset the default login in the shared base setup.This base is now reused across the suite, but
setUp()only clearsrequest.formand resets roles. Some tests switch principals withlogin(..., "admin")/logout(), so if that shared security context persists between methods the permission checks become order-dependent. Re-logging the default test user here would make the isolation explicit.Small base-setup reset
from imio.esign.testing import IMIO_ESIGN_INTEGRATION_TESTING +from plone.app.testing import login from plone.app.testing import setRoles from plone.app.testing import TEST_USER_ID +from plone.app.testing import TEST_USER_NAME @@ def setUp(self): self.portal = self.layer["portal"] self.request = self.layer["request"] self.request.form.clear() setRoles(self.portal, TEST_USER_ID, ["Manager"]) + login(self.portal, TEST_USER_NAME)Also applies to: 19-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/tests/base.py` around lines 3 - 5, The shared test base's setUp currently only clears request.form and resets roles, which can leave a switched-in principal from a previous test; update setUp to explicitly re-login the default test user by calling the plone testing login with TEST_USER_ID (and then restore roles via setRoles as needed) so the security context is deterministic; locate the setUp method in the base test class and add a login(self.portal, TEST_USER_ID) (importing login if missing) immediately after the request/roles reset to ensure each test starts with the default principal.src/imio/esign/testing.py (1)
77-82: Own this fixture instead of readingcollective.iconifiedcategory's test data.
setUpPloneSite()now depends oncollective.iconifiedcategory/tests/icône1.png, which is an internal test asset. If that file is missing from the installed distribution, the whole layer dies before any test runs. Please move the icon into this repo (or build a tiny blob inline) so the suite only depends on repo-owned fixtures.Minimal path change
- icon_path = os.path.join( - os.path.dirname(collective.iconifiedcategory.__file__), - "tests", - u"icône1.png", - ) + icon_path = os.path.join(os.path.dirname(__file__), "tests", "icon.png")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/testing.py` around lines 77 - 82, The fixture currently reads an external test asset via icon_path in setUpPloneSite(), which makes the layer depend on collective.iconifiedcategory; instead, add and use a repo-owned fixture: either add the icône1.png into this repository (e.g., tests/assets/) and change the icon_path construction to join os.path.dirname(__file__) with that local assets path (update the icon_path variable usage), or embed a small PNG as a base64 blob in testing.py and decode it to bytes where the file is opened (replace the with open(icon_path, "rb") block to use the in-repo file or decoded bytes). Ensure references are to the icon_path variable and the setUpPloneSite() flow so the layer no longer depends on external package test data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/esign/testing.py`:
- Around line 77-82: The fixture currently reads an external test asset via
icon_path in setUpPloneSite(), which makes the layer depend on
collective.iconifiedcategory; instead, add and use a repo-owned fixture: either
add the icône1.png into this repository (e.g., tests/assets/) and change the
icon_path construction to join os.path.dirname(__file__) with that local assets
path (update the icon_path variable usage), or embed a small PNG as a base64
blob in testing.py and decode it to bytes where the file is opened (replace the
with open(icon_path, "rb") block to use the in-repo file or decoded bytes).
Ensure references are to the icon_path variable and the setUpPloneSite() flow so
the layer no longer depends on external package test data.
In `@src/imio/esign/tests/base.py`:
- Around line 3-5: The shared test base's setUp currently only clears
request.form and resets roles, which can leave a switched-in principal from a
previous test; update setUp to explicitly re-login the default test user by
calling the plone testing login with TEST_USER_ID (and then restore roles via
setRoles as needed) so the security context is deterministic; locate the setUp
method in the base test class and add a login(self.portal, TEST_USER_ID)
(importing login if missing) immediately after the request/roles reset to ensure
each test starts with the default principal.
In `@src/imio/esign/tests/test_utils.py`:
- Around line 474-479: The test currently only asserts the JSON payload from
create_external_session; add an assertion that mock_post.call_args[1]["files"]
is the expected multipart shape (a list of ("files", (filename, content,
content_type)) tuples) to confirm duplicate filenames survive and the code sends
a list of repeated "files" tuples; update the assertion next to the existing
payload check in test_utils.py (around the create_external_session call) to
validate mock_post.call_args[1]["files"] is a list and contains the expected
repeated filename entries (and apply the same additional files assertion at the
other two test locations referenced in the comment).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b922db63-25f7-43c7-9298-f1c3b7dd77cd
📒 Files selected for processing (10)
src/imio/esign/events.pysrc/imio/esign/testing.pysrc/imio/esign/tests/base.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_browser_settings.pysrc/imio/esign/tests/test_browser_views.pysrc/imio/esign/tests/test_services.pysrc/imio/esign/tests/test_setup.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
✅ Files skipped from review due to trivial changes (2)
- src/imio/esign/tests/test_setup.py
- src/imio/esign/events.py
chris-adam
force-pushed
the
PARAF-360/esign_tests
branch
from
27fc56f to
bbfe323
Compare
Contributor
Author
|
Je vais rebase et merge cette PR parce que ça me bloque pour pouvoir continuer le dev |
chris-adam
force-pushed
the
PARAF-360/esign_tests
branch
from
bbfe323 to
5b25970
Compare
chris-adam
force-pushed
the
PARAF-360/esign_tests
branch
from
5b25970 to
31e422c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/imio/esign/tests/test_services.py (2)
155-170: ⚡ Quick winFully assert the
returnstuple shape (including datetime slot).The comment says
(code, db_state, value, message, datetime), but the test only validates the first four fields. Add a shape check so a dropped datetime value is caught.Suggested patch
entry = srtn["returns"][0] + self.assertEqual(len(entry), 5) self.assertEqual(entry[0], 23) self.assertEqual(entry[1], "completed") self.assertEqual(entry[2], {"info": "ok"}) self.assertEqual(entry[3], "All done") + self.assertIsNotNone(entry[4])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/tests/test_services.py` around lines 155 - 170, The test currently checks only the first four slots of the returns tuple from add_files_to_session (variable srtn and entry) but omits the datetime slot; update the assertions for srtn["returns"][0] (entry) to assert the tuple length is 5 and that the fifth element is a datetime (e.g., an instance of datetime.datetime) before existing assertions so a missing or dropped datetime will fail; locate the checks around add_files_to_session and self._reply in test_services.py and add the length + type assertion immediately after assigning entry.
63-77: ⚡ Quick winAdd one end-to-end unauthorized
reply()assertion.
test_reply()currently goes through_reply(), which forcesverify_auth_token=True, so the auth gate inreply()is never exercised in this test method. Add one negative-auth call to catch regressions wherereply()stops enforcing authorization.Suggested patch
def test_reply(self): """reply() validates auth/input, processes all feedback codes, updates session state.""" annex1 = self.portal["folder0"]["annex2"] + + # unauthorized request should be rejected by reply() + self.request.set("BODY", json.dumps({"app_session_id": self.sign_id, "code": 21})) + with patch("imio.esign.services.external_session_feedback.verify_auth_token", return_value=False): + self._make_service().reply() + self.assertNotEqual(self.request.response.getStatus(), 200) + self.request.response.setStatus(200) # missing app_session_id result = self._reply({"code": 21})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/tests/test_services.py` around lines 63 - 77, In test_reply(), add an end-to-end unauthorized assertion that calls the real reply() flow instead of the helper _reply() (which forces verify_auth_token=True); invoke reply() (or the service endpoint used by reply()) with a payload like {"app_session_id": <valid-or-sample>, "code": 21} but without any auth token and assert the response status is 401/403 and the response message indicates unauthorized (or contains "auth" / "unauthorized"); ensure you place this before other state-changing assertions and reference the existing test method test_reply, the helper _reply, and the reply() auth gate (verify_auth_token) so the test fails if authorization stops being enforced.src/imio/esign/tests/test_actions.py (2)
34-38: ⚡ Quick winFactor duplicated test setup into a shared helper/base method.
The repeated user/signer/fixture initialization across three classes will drift over time. Moving this into
BaseEsignTest(or a local helper) would reduce maintenance overhead.Also applies to: 85-89, 115-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/tests/test_actions.py` around lines 34 - 38, Extract the duplicated test setup (api.user.create(...), setting self.folder, self.annexes, self.signers, and instantiating RemoveItemFromSessionView) into a reusable helper method on BaseEsignTest (e.g., def setup_common_fixtures(self): ...) or a module-level helper, then replace the duplicated blocks in each test class with a single call to that helper; ensure the helper returns or assigns the same attributes (folder, annexes, signers, view) so callers use the identical names used in the tests (self.folder, self.annexes, self.signers, self.view) and update imports if you move the helper to a separate module.
160-167: ⚡ Quick winAvoid hardcoded absolute URLs in expected HTML assertions.
The assertions currently pin
http://nohost/plone/...; deriving expected URLs from fixture objects would make tests less environment-coupled while keeping the same intent.♻️ Suggested adjustment
- self.assertEqual( - self.view._uid_to_link(uid), - u"<a href='http://nohost/plone/folder0/view' title='/plone/folder0'>Folder 0</a>", - ) + expected_folder_link = u"<a href='{}/view' title='{}'>{}</a>".format( + self.folder.absolute_url(), self.folder.absolute_url_path(), self.folder.Title() + ) + self.assertEqual(self.view._uid_to_link(uid), expected_folder_link)Also applies to: 221-235
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/tests/test_actions.py` around lines 160 - 167, Tests assert absolute HTML links using a hardcoded "http://nohost/plone/..." which couples them to an environment; update the assertions in tests referencing _uid_to_link (and the similar assertions at lines 221-235) to build the expected URL from the test fixtures instead (e.g., use self.portal.absolute_url() or self.view.context.absolute_url() / request.getURL() to derive the base and then append the relative path) and compare against that generated URL in the expected <a> tag or <span> output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/imio/esign/tests/test_actions.py`:
- Around line 34-38: Extract the duplicated test setup (api.user.create(...),
setting self.folder, self.annexes, self.signers, and instantiating
RemoveItemFromSessionView) into a reusable helper method on BaseEsignTest (e.g.,
def setup_common_fixtures(self): ...) or a module-level helper, then replace the
duplicated blocks in each test class with a single call to that helper; ensure
the helper returns or assigns the same attributes (folder, annexes, signers,
view) so callers use the identical names used in the tests (self.folder,
self.annexes, self.signers, self.view) and update imports if you move the helper
to a separate module.
- Around line 160-167: Tests assert absolute HTML links using a hardcoded
"http://nohost/plone/..." which couples them to an environment; update the
assertions in tests referencing _uid_to_link (and the similar assertions at
lines 221-235) to build the expected URL from the test fixtures instead (e.g.,
use self.portal.absolute_url() or self.view.context.absolute_url() /
request.getURL() to derive the base and then append the relative path) and
compare against that generated URL in the expected <a> tag or <span> output.
In `@src/imio/esign/tests/test_services.py`:
- Around line 155-170: The test currently checks only the first four slots of
the returns tuple from add_files_to_session (variable srtn and entry) but omits
the datetime slot; update the assertions for srtn["returns"][0] (entry) to
assert the tuple length is 5 and that the fifth element is a datetime (e.g., an
instance of datetime.datetime) before existing assertions so a missing or
dropped datetime will fail; locate the checks around add_files_to_session and
self._reply in test_services.py and add the length + type assertion immediately
after assigning entry.
- Around line 63-77: In test_reply(), add an end-to-end unauthorized assertion
that calls the real reply() flow instead of the helper _reply() (which forces
verify_auth_token=True); invoke reply() (or the service endpoint used by
reply()) with a payload like {"app_session_id": <valid-or-sample>, "code": 21}
but without any auth token and assert the response status is 401/403 and the
response message indicates unauthorized (or contains "auth" / "unauthorized");
ensure you place this before other state-changing assertions and reference the
existing test method test_reply, the helper _reply, and the reply() auth gate
(verify_auth_token) so the test fails if authorization stops being enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54ab4a73-f0f0-40a4-a9b1-172be93bba0c
⛔ Files ignored due to path filters (1)
src/imio/esign/tests/icône1.pngis excluded by!**/*.png
📒 Files selected for processing (10)
src/imio/esign/events.pysrc/imio/esign/testing.pysrc/imio/esign/tests/base.pysrc/imio/esign/tests/test_actions.pysrc/imio/esign/tests/test_browser_settings.pysrc/imio/esign/tests/test_browser_views.pysrc/imio/esign/tests/test_services.pysrc/imio/esign/tests/test_setup.pysrc/imio/esign/tests/test_utils.pysrc/imio/esign/utils.py
✅ Files skipped from review due to trivial changes (4)
- src/imio/esign/events.py
- src/imio/esign/tests/base.py
- src/imio/esign/tests/test_browser_settings.py
- src/imio/esign/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/imio/esign/testing.py
- src/imio/esign/tests/test_utils.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit