Conversation
There was a problem hiding this comment.
Pull request overview
Updates the test suite and project configuration to run under pytest, aligns tests with recent internal API changes (job manager now stores Job objects in a dict), and fixes a CLI short-option collision for patchipa.
Changes:
- Add pytest/pytest-cov as optional “test” dependencies and introduce pytest discovery/options in
pyproject.toml. - Update multiple tests to be tolerant of
tabulateformatting differences vianormalize_table_whitespace(), and update job-related tests to use theJobclass/dict-backed state. - Fix duplicate
-bflag by changingpatchipa --bundle-idshort option to-B.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds pytest/pytest-cov (and coverage/iniconfig) and records the new objection version metadata. |
| pyproject.toml | Adds test extras and pytest configuration under [tool.pytest.ini_options]. |
| objection/console/cli.py | Resolves patchipa short-option collision by changing --bundle-id to -B. |
| tests/helpers.py | Adds normalize_table_whitespace() helper to reduce table-format brittleness across environments. |
| tests/state/test_jobs.py | Updates job manager tests for dict-backed storage and Job objects. |
| tests/commands/test_ui.py | Fixes platform comparison semantics and mock assertions. |
| tests/commands/test_memory.py | Normalizes table output comparisons for module/export listings. |
| tests/commands/test_jobs.py | Updates job command tests for dict-backed job state and new output format. |
| tests/commands/test_filemanager.py | Adjusts expected messages and normalizes table comparisons; updates download helper call signatures. |
| tests/commands/ios/test_nsurlcredentialstorage.py | Normalizes table output comparisons. |
| tests/commands/ios/test_keychain.py | Normalizes keychain table comparisons and updates an expected validation message. |
| tests/commands/ios/test_cookies.py | Normalizes table output comparisons. |
| tests/commands/ios/test_bundles.py | Normalizes table output comparisons across bundle listings. |
| tests/commands/android/test_keystore.py | Normalizes table output comparisons for keystore entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [tool.pytest.ini_options] | ||
| testpaths = ["tests"] | ||
| python_files = "test_*.py" | ||
| python_classes = "Test*" | ||
| python_functions = "test_*" | ||
| addopts = "-v --strict-markers" |
There was a problem hiding this comment.
PR description mentions adding uv run pytest to a uv config, but this change set only adds pytest dependencies and pytest ini options; there is no tool.uv/uv config entry or script/Makefile update wiring uv run pytest. If a uv configuration change is intended, it looks missing from this PR (or the description should be updated).
| expected_output = """Job ID Type Name | ||
| ------ ---- ---- | ||
| """ |
There was a problem hiding this comment.
The expected tabulate output includes a leading space on the separator row (" ------ ...."). tabulate(..., tablefmt='simple') output does not typically prefix rows with an extra leading space, so this assertion is likely to fail depending on tabulate/version/column alignment. Consider either removing the leading space in the expected string or comparing via normalize_table_whitespace() (as done in other table-based tests).
| expected_outut = """Job ID Type Name | ||
| ------ ---- --------------------- | ||
| 123456 hook ios-jailbreak-disable | ||
| """ |
There was a problem hiding this comment.
Similar to the empty-table test, this expected output string has leading spaces before the separator and data rows. This is brittle across tabulate versions and may not match the actual output. Consider comparing normalized output (e.g., normalize_table_whitespace(output)), or ensure the expected string matches tabulate's exact formatting without extra leading spaces.
| def test_cant_find_job_by_uuid(self): | ||
| # Attempting to kill a job that doesn't exist just removes it from state | ||
| # If it wasn't there, nothing happens | ||
| kill(['123']) | ||
| # Job was not in manager, so nothing happened | ||
| self.assertEqual(len(job_manager_state.jobs), 0) |
There was a problem hiding this comment.
This test calls kill(['123']) without capturing stdout/stderr. In the current implementation, remove_job() prints an error via click.secho when the job is missing, so the test will emit output but not assert the behavior. Consider using capture(kill, ['123']) and asserting the error message (or patching click.secho) to make the test verify the intended behavior.
| @mock.patch('objection.state.connection.state_connection.get_api') | ||
| def test_kills_job_by_uuid(self, mock_api): | ||
| kill('foo') | ||
| # Add a job and then kill it | ||
| mock_handle = mock.MagicMock() | ||
| job = Job('test', 'hook', mock_handle, 123) | ||
| job_manager_state.add_job(job) | ||
| self.assertEqual(len(job_manager_state.jobs), 1) | ||
|
|
||
| kill(['123']) | ||
|
|
||
| self.assertEqual(len(job_manager_state.jobs), 0) |
There was a problem hiding this comment.
Job.end() for job_type == 'hook' calls state_connection.get_api().jobs_kill(self.uuid), but this test only asserts that the job was removed from local state. Consider also asserting that mock_api.return_value.jobs_kill was called with the expected UUID so the test validates the external side effect too.
| def test_adds_jobs(self): | ||
| job_manager_state.add_job('foo') | ||
| job = Job('foo', 'test', None) | ||
| job_manager_state.add_job(job) | ||
|
|
||
| self.assertEqual(len(job_manager_state.jobs), 1) | ||
|
|
||
| def test_removes_jobs(self): | ||
| job_manager_state.add_job('foo') | ||
| job_manager_state.add_job('bar') | ||
| job1 = Job('foo', 'test', None) | ||
| job2 = Job('bar', 'test', None) | ||
| job_manager_state.add_job(job1) | ||
| job_manager_state.add_job(job2) | ||
|
|
||
| job_manager_state.remove_job('foo') | ||
| job_manager_state.remove_job('bar') | ||
| job_manager_state.remove_job(job1.uuid) | ||
| job_manager_state.remove_job(job2.uuid) |
There was a problem hiding this comment.
These tests create Job(..., job_type='test', ...), but Job.end() only supports job_type values "script" and "hook"; any other value prints an "Unknown job type" error when the job is removed. To avoid noisy output and ensure the test reflects real usage, use job_type='script' with a mock handle that provides unload(), or use job_type='hook' while patching state_connection.get_api().jobs_kill.
1 participant
uv run pytestto uv config.-boptions of--patch-ipa