Fix publish --build in non-interactive mode when dist already has artifacts#10768
Fix publish --build in non-interactive mode when dist already has artifacts#10768SergioChan wants to merge 3 commits intopython-poetry:mainfrom
Conversation
Reviewer's GuideEnsures Sequence diagram for publish_build_flow_in_interactive_and_non_interactive_modessequenceDiagram
actor User
participant PublishCommand
participant IO
participant Publisher
User->>PublishCommand: run publish --build
PublishCommand->>Publisher: inspect files
Publisher-->>PublishCommand: files list
alt build_option_enabled
PublishCommand->>PublishCommand: check publisher.files
alt files_exist_and_interactive
PublishCommand->>IO: is_interactive()
IO-->>PublishCommand: true
PublishCommand->>User: confirm(Build anyway?)
alt user_confirms
PublishCommand->>Publisher: build()
Publisher-->>PublishCommand: build complete
PublishCommand->>Publisher: publish()
Publisher-->>PublishCommand: publish complete
else user_aborts
PublishCommand->>User: line_error(Aborted!)
end
else non_interactive_or_no_files
PublishCommand->>IO: is_interactive()
IO-->>PublishCommand: false or not_called
PublishCommand->>Publisher: build()
Publisher-->>PublishCommand: build complete
PublishCommand->>Publisher: publish()
Publisher-->>PublishCommand: publish complete
end
else build_option_disabled
PublishCommand->>Publisher: publish()
Publisher-->>PublishCommand: publish complete
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/console/commands/test_publish.py" line_range="220-229" />
<code_context>
assert "- Uploading simple_project-1.2.3-py2.py3-none-any.whl" in error
+
+
+def test_publish_build_no_interaction_skips_confirmation(
+ app_tester: ApplicationTester, mocker: MockerFixture
+) -> None:
+ confirm = mocker.patch("poetry.console.commands.publish.PublishCommand.confirm")
+ command_call = mocker.patch("poetry.console.commands.publish.PublishCommand.call")
+ publisher_publish = mocker.patch("poetry.publishing.Publisher.publish")
+
+ exit_code = app_tester.execute("publish --build --no-interaction --dry-run")
+
+ assert exit_code == 0
+ confirm.assert_not_called()
+ command_call.assert_called_once_with("build", args="--output dist")
+ assert publisher_publish.call_count == 1
</code_context>
<issue_to_address>
**issue (testing):** Test does not simulate pre-existing dist artifacts, so it doesn’t cover the regression scenario described in the PR
Because `Publisher.publish` is mocked and `publisher.files` is never set, `publisher.files` remains falsy and the `if publisher.files and self.io.is_interactive()` branch is never hit. This test only proves that `--no-interaction` skips `confirm` when there are no existing artifacts. To actually cover the regression, ensure the `Publisher` used by `PublishCommand` has a non-empty `files` list (e.g. via a fixture, a real `Publisher`, or by patching the constructor and setting `.files`), so the scenario with pre-existing `dist` artifacts is exercised.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_publish_build_no_interaction_skips_confirmation( | ||
| app_tester: ApplicationTester, mocker: MockerFixture | ||
| ) -> None: | ||
| confirm = mocker.patch("poetry.console.commands.publish.PublishCommand.confirm") | ||
| command_call = mocker.patch("poetry.console.commands.publish.PublishCommand.call") | ||
| publisher_publish = mocker.patch("poetry.publishing.Publisher.publish") | ||
|
|
||
| exit_code = app_tester.execute("publish --build --no-interaction --dry-run") | ||
|
|
||
| assert exit_code == 0 |
There was a problem hiding this comment.
issue (testing): Test does not simulate pre-existing dist artifacts, so it doesn’t cover the regression scenario described in the PR
Because Publisher.publish is mocked and publisher.files is never set, publisher.files remains falsy and the if publisher.files and self.io.is_interactive() branch is never hit. This test only proves that --no-interaction skips confirm when there are no existing artifacts. To actually cover the regression, ensure the Publisher used by PublishCommand has a non-empty files list (e.g. via a fixture, a real Publisher, or by patching the constructor and setting .files), so the scenario with pre-existing dist artifacts is exercised.
|
Pushed a follow-up commit to tighten the regression coverage:
Validation run:
|
|
Addressed in 5673d45.
Validation run locally:
|
Pull Request Check List
Resolves: #10760
Summary
publish --build --no-interactiondoes not callconfirmand still proceeds.Testing
poetry run pytest tests/console/commands/test_publish.py -k "no_interaction or dist_dir_and_build_options"Summary by Sourcery
Skip interactive confirmation when publishing with build in non-interactive mode and add regression coverage for this behavior.
Bug Fixes:
Tests:
publish --build --no-interactionskips confirmation but still builds and publishes.