diff --git a/src/deadline/client/ui/controllers/_deadline_controller.py b/src/deadline/client/ui/controllers/_deadline_controller.py index 54f34bf2a..feecd3157 100644 --- a/src/deadline/client/ui/controllers/_deadline_controller.py +++ b/src/deadline/client/ui/controllers/_deadline_controller.py @@ -84,6 +84,10 @@ class DeadlineUIController(QObject): queues_updated: Emitted when queue list is updated. Args: [(name, queue_id), ...] storage_profiles_updated: Emitted when storage profiles are updated. queue_parameters_updated: Emitted when queue parameters are loaded. + queue_parameters_load_succeeded: Emitted only when a queue parameter fetch + completes successfully (queue_parameters_updated also fires with [] on + fetch errors and for clearing emissions). Args: [parameter, ...] — may + be empty for a queue that genuinely has no queue parameters. farms_loading: Emitted when farm loading state changes. Args: bool queues_loading: Emitted when queue loading state changes. Args: bool storage_profiles_loading: Emitted when storage profile loading state changes. @@ -101,6 +105,10 @@ class DeadlineUIController(QObject): queues_updated = Signal(list) storage_profiles_updated = Signal(list) queue_parameters_updated = Signal(list) + # Success-only companion to queue_parameters_updated: not emitted on fetch + # error or for clearing emissions, so consumers that must act only on real, + # successful loads (e.g. CLI --parameter validation) can connect here. + queue_parameters_load_succeeded = Signal(list) # Emitted after a farm or queue selection has been persisted, so a host dialog # can reload queue parameters and refresh its Submit button state. @@ -495,6 +503,7 @@ def _on_queue_parameters_success(self, parameters: List[JobParameter]) -> None: """Handle successful queue parameters fetch.""" self.queue_parameters_loading.emit(False) self.queue_parameters_updated.emit(parameters) + self.queue_parameters_load_succeeded.emit(parameters) def _on_queue_parameters_error(self, error: BaseException) -> None: """Handle queue parameters fetch error.""" diff --git a/src/deadline/client/ui/job_bundle_submitter.py b/src/deadline/client/ui/job_bundle_submitter.py index d46d51618..65e3beb11 100644 --- a/src/deadline/client/ui/job_bundle_submitter.py +++ b/src/deadline/client/ui/job_bundle_submitter.py @@ -411,20 +411,51 @@ def on_create_job_bundle_callback( ) if job_parameters: - # We want to validate the job parameters after the queue parameters are loaded. - # Connect a parameter validation function to the queue parameter loading completion - def validate_parameters_after_queue_load(refresh_id: int, queue_parameters: list): + # The controller is a global singleton that outlives this dialog, and its + # queue_parameters_updated signal also fires with [] to clear stale state + # (farm/queue switch, fetch error, nothing selected) — indistinguishable + # from a queue that genuinely has zero queue parameters. Connect to the + # success-only queue_parameters_load_succeeded signal instead, so we only + # validate against a real, successful load (which may legitimately be + # empty) and keep waiting through clears and transient fetch errors. + # Validate single-shot, then disconnect so the closure over + # submitter_dialog can't fire against a closed dialog later. + controller = submitter_dialog.shared_job_settings._controller + # Mutable cell so both closures below share the connection state. Guards + # the double-disconnect (validation ran, then the dialog is destroyed), + # which PySide6 reports with a RuntimeWarning rather than an exception. + connected = [False] + + def disconnect_validation_callback(): + if not connected[0]: + return + connected[0] = False + try: + controller.queue_parameters_load_succeeded.disconnect( + validate_parameters_after_queue_load + ) + except (TypeError, RuntimeError): + # Some bindings raise instead of warn when already disconnected. + pass + + def validate_parameters_after_queue_load(queue_parameters: list): """Validate CLI parameters against loaded queue parameters and set parameter values""" + disconnect_validation_callback() if not _validate_and_warn_about_parameters( job_parameters, initial_settings.parameters, queue_parameters, submitter_dialog ): - # User chose to cancel, close the dialog + # User cancelled at the validation warning. submitter_dialog.close() - # Connect to the queue parameters update signal - submitter_dialog.shared_job_settings._queue_parameters_update.connect( - validate_parameters_after_queue_load - ) + # Validate CLI params once the controller successfully loads queue params. + controller.queue_parameters_load_succeeded.connect(validate_parameters_after_queue_load) + connected[0] = True + # If the dialog goes away before any load succeeds, tear the connection down. + # The dialog does not set WA_DeleteOnClose, so an ordinary close never emits + # destroyed — hook finished (emitted on accept/reject/close of a visible + # dialog) for the close path, and destroyed for deletion without done(). + submitter_dialog.finished.connect(lambda _result: disconnect_validation_callback()) + submitter_dialog.destroyed.connect(disconnect_validation_callback) submitter_dialog.show() return submitter_dialog diff --git a/test/unit/deadline_client/ui/controllers/test_deadline_controller.py b/test/unit/deadline_client/ui/controllers/test_deadline_controller.py index 7d4ec11a9..898e37704 100644 --- a/test/unit/deadline_client/ui/controllers/test_deadline_controller.py +++ b/test/unit/deadline_client/ui/controllers/test_deadline_controller.py @@ -567,6 +567,72 @@ def test_select_storage_profile_persists_without_cascade( mock_api.list_storage_profiles_for_queue.assert_not_called() mock_api.get_queue_parameter_definitions.assert_not_called() + @patch("deadline.client.ui.controllers._deadline_controller.api") + def test_queue_params_success_emits_load_succeeded( + self, mock_api, qtbot, fresh_deadline_config + ): + """A successful fetch emits queue_parameters_load_succeeded with the parameters, + even when the queue genuinely has zero queue parameters.""" + controller = DeadlineUIController.getInstance() + mock_api.get_queue_parameter_definitions.return_value = [] + + succeeded = [] + updated = [] + controller.queue_parameters_load_succeeded.connect( + lambda x: succeeded.append(x), _QueuedConnection + ) + controller.queue_parameters_updated.connect(lambda x: updated.append(x), _QueuedConnection) + + controller.refresh_queue_parameters(farm_id="farm-123", queue_id="queue-456") + + qtbot.waitUntil(lambda: len(succeeded) > 0, timeout=2000) + assert succeeded[0] == [] + assert updated[0] == [] + + @patch("deadline.client.ui.controllers._deadline_controller.api") + def test_queue_params_error_does_not_emit_load_succeeded( + self, mock_api, qtbot, fresh_deadline_config + ): + """A failed fetch emits queue_parameters_updated([]) but NOT load_succeeded, so + success-only consumers can distinguish an error from an empty-but-real load.""" + controller = DeadlineUIController.getInstance() + mock_api.get_queue_parameter_definitions.side_effect = RuntimeError("boom") + + succeeded = [] + updated = [] + controller.queue_parameters_load_succeeded.connect( + lambda x: succeeded.append(x), _QueuedConnection + ) + controller.queue_parameters_updated.connect(lambda x: updated.append(x), _QueuedConnection) + + controller.refresh_queue_parameters(farm_id="farm-123", queue_id="queue-456") + + qtbot.waitUntil(lambda: len(updated) > 0, timeout=2000) + assert updated[0] == [] + assert succeeded == [] + + @patch("deadline.client.ui.controllers._deadline_controller.api") + def test_clearing_emission_does_not_emit_load_succeeded( + self, mock_api, qtbot, fresh_deadline_config + ): + """refresh with no farm/queue selected clears via queue_parameters_updated([]) without + emitting load_succeeded.""" + controller = DeadlineUIController.getInstance() + + succeeded = [] + updated = [] + controller.queue_parameters_load_succeeded.connect( + lambda x: succeeded.append(x), _QueuedConnection + ) + controller.queue_parameters_updated.connect(lambda x: updated.append(x), _QueuedConnection) + + controller.refresh_queue_parameters(farm_id="", queue_id="") + + qtbot.waitUntil(lambda: len(updated) > 0, timeout=2000) + assert updated[0] == [] + assert succeeded == [] + mock_api.get_queue_parameter_definitions.assert_not_called() + def test_shutdown_cancels_operations(self, qtbot): """Test that shutdown cancels pending operations.""" controller = DeadlineUIController.getInstance() diff --git a/test/unit/deadline_client/ui/gui/test_gui_submitter_cli_parameters.py b/test/unit/deadline_client/ui/gui/test_gui_submitter_cli_parameters.py new file mode 100644 index 000000000..a256d96ee --- /dev/null +++ b/test/unit/deadline_client/ui/gui/test_gui_submitter_cli_parameters.py @@ -0,0 +1,323 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""GUI test for the ``bundle gui-submit --parameter`` validation wiring. + +Verifies ``show_job_bundle_submitter`` connects the CLI ``--parameter`` validation callback to +the controller's ``queue_parameters_updated`` signal. Uses a real ``SharedJobSettingsWidget`` +(not a MagicMock) so the connection is actually resolved — a mocked dialog would let a broken +connect pass silently. +""" + +import os + +from unittest.mock import MagicMock, patch + +import pytest + +from qtpy.QtWidgets import QDialog # type: ignore + +from deadline.client.ui.controllers._deadline_controller import DeadlineUIController +from deadline.client.ui.controllers._thread_pool import DeadlineThreadPool +from deadline.client.ui.dataclasses import JobBundleSettings +from deadline.client.ui.job_bundle_submitter import show_job_bundle_submitter +from deadline.client.ui.widgets.shared_job_settings_tab import SharedJobSettingsWidget + +MODULE = "deadline.client.ui.job_bundle_submitter" + + +@pytest.fixture(autouse=True) +def _reset_singletons(): + """Reset UI singletons before/after each test so the controller is clean.""" + DeadlineUIController.resetInstance() + DeadlineThreadPool.reset() + yield + DeadlineUIController.resetInstance() + DeadlineThreadPool.shutdown(wait_for_done=True, timeout_ms=2000) + DeadlineThreadPool.reset() + + +def _make_bundle(tmp_path): + bundle_dir = str(tmp_path / "bundle") + os.makedirs(bundle_dir) + with open(os.path.join(bundle_dir, "template.yaml"), "w") as f: + f.write("name: Bundle Job\nsteps: []\n") + return bundle_dir + + +class TestGuiSubmitCliParameterValidationWiring: + """The --parameter validation callback is wired to the real queue-parameters signal.""" + + def _run(self, qtbot, tmp_path, *, job_parameters, validate_side_effect, emit_after=None): + """Drive show_job_bundle_submitter with a REAL SharedJobSettingsWidget standing in for + the dialog's shared_job_settings. No farm/queue is configured (fresh_deadline_config), + so the widget does not kick off a background load. + + If ``emit_after`` is provided, controller signals are emitted *inside* the patched + context so the connected validation callback runs against the mocked + ``_validate_and_warn_about_parameters``. It is a list of (kind, payload) tuples + emitted in sequence, mirroring the controller's real emission patterns + (see DeadlineUIController._on_queue_parameters_success/_error and the + clearing emits in select_farm/refresh_queue_parameters): + + - ("load", payload): a successful fetch — loading(True), loading(False), + queue_parameters_updated(payload), queue_parameters_load_succeeded(payload). + - ("error", _): a failed fetch — loading(True), loading(False), + queue_parameters_updated([]); load_succeeded is NOT emitted. + - ("clear", payload): a bare queue_parameters_updated(payload) with no loading + prefix, as emitted on farm/queue switch or when nothing is selected. + + Returns (dialog, widget, validate_mock).""" + bundle_dir = _make_bundle(tmp_path) + + settings = JobBundleSettings(input_job_bundle_dir=bundle_dir, name="n") + real_widget = SharedJobSettingsWidget( + initial_settings=settings, initial_shared_parameter_values={} + ) + qtbot.addWidget(real_widget) + + class FakeDialog(QDialog): + # A real QDialog so the production code's teardown hooks (``finished`` + # on ordinary close, ``destroyed`` on deletion) behave exactly as they + # do for SubmitJobToDeadlineDialog, which also does not set + # WA_DeleteOnClose. + def __init__(self, **kwargs): + super().__init__() + self.shared_job_settings = real_widget + self.closed = False + + def show(self): + pass + + def close(self): + self.closed = True + return super().close() + + template = {"name": "Bundle Job", "steps": []} + validate_mock = MagicMock(side_effect=validate_side_effect) + + with ( + patch(f"{MODULE}.validate_directory_symlink_containment"), + patch( + f"{MODULE}.read_yaml_or_json_object", + side_effect=lambda _dir, name, *a, **k: template if name == "template" else None, + ), + patch(f"{MODULE}.read_job_bundle_parameters", return_value=[]), + patch(f"{MODULE}.run_pre_gui_hooks", return_value={}), + patch(f"{MODULE}.SubmitJobToDeadlineDialog", side_effect=FakeDialog), + patch(f"{MODULE}.QApplication"), + patch(f"{MODULE}.QMessageBox"), + patch(f"{MODULE}._get_setting", side_effect=lambda name, config=None: "false"), + patch(f"{MODULE}._config_file") as cfg, + patch(f"{MODULE}._validate_and_warn_about_parameters", validate_mock), + ): + cfg.str2bool.side_effect = lambda v: str(v).lower() == "true" + dialog = show_job_bundle_submitter( + input_job_bundle_dir=bundle_dir, job_parameters=job_parameters + ) + # Emitting inside the patched block keeps _validate_and_warn_about_parameters mocked + # when the connected callback fires. + if emit_after is not None: + controller = real_widget._controller + for kind, payload in emit_after: + if kind == "load": + # A successful fetch (_on_queue_parameters_success). + controller.queue_parameters_loading.emit(True) + controller.queue_parameters_loading.emit(False) + controller.queue_parameters_updated.emit(payload) + controller.queue_parameters_load_succeeded.emit(payload) + elif kind == "error": + # A failed fetch (_on_queue_parameters_error): no load_succeeded. + controller.queue_parameters_loading.emit(True) + controller.queue_parameters_loading.emit(False) + controller.queue_parameters_updated.emit([]) + else: + assert kind == "clear" + controller.queue_parameters_updated.emit(payload) + return dialog, real_widget, validate_mock + + def test_parameter_path_does_not_raise_attribute_error( + self, qtbot, fresh_deadline_config, tmp_path + ): + """Wiring the --parameter path must not raise AttributeError (the C6 hard crash).""" + dialog, _widget, _validate = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + ) + assert dialog is not None + + def test_queue_parameters_update_invokes_validator_with_param_list( + self, qtbot, fresh_deadline_config, tmp_path + ): + """A completed queue-parameter load runs the validator with the loaded list.""" + queue_parameters = [{"name": "CondaChannels", "type": "STRING"}] + _dialog, _widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + emit_after=[("load", queue_parameters)], + ) + + validate_mock.assert_called_once() + # Signature: (job_parameters, job_template_parameters, queue_parameters, parent_widget) + assert validate_mock.call_args.args[2] == queue_parameters + + def test_validator_cancel_closes_dialog(self, qtbot, fresh_deadline_config, tmp_path): + """When the validator returns False (user cancels), the dialog is closed.""" + dialog, _widget, _validate = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: False, + emit_after=[("load", [{"name": "Foo"}])], + ) + + assert dialog.closed is True + + def test_clearing_emission_does_not_invoke_validator( + self, qtbot, fresh_deadline_config, tmp_path + ): + """A clearing emission ([] with no loading prefix) — e.g. farm/queue switch or nothing + selected — must not run validation (which would spuriously flag queue params as + unrecognized).""" + _dialog, _widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + emit_after=[("clear", [])], + ) + + validate_mock.assert_not_called() + + def test_empty_but_real_load_still_invokes_validator( + self, qtbot, fresh_deadline_config, tmp_path + ): + """A queue that genuinely has zero queue parameters still validates: a completed load + (loading True -> False, then updated([])) runs the validator so an unrecognized CLI + --parameter is flagged rather than slipping through.""" + _dialog, _widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + emit_after=[("load", [])], + ) + + validate_mock.assert_called_once() + assert validate_mock.call_args.args[2] == [] + + def test_failed_load_does_not_invoke_validator(self, qtbot, fresh_deadline_config, tmp_path): + """A failed fetch (e.g. transient ResourceNotFoundException during a profile switch) + must not validate against []; the callback keeps waiting for a successful load.""" + queue_parameters = [{"name": "CondaChannels", "type": "STRING"}] + _dialog, _widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + emit_after=[ + ("error", None), + ("load", queue_parameters), + ], + ) + + # Only the successful load validates, with its real parameter list. + validate_mock.assert_called_once() + assert validate_mock.call_args.args[2] == queue_parameters + + def test_validator_runs_single_shot_on_first_successful_load( + self, qtbot, fresh_deadline_config, tmp_path + ): + """Validation waits through clearing emissions, runs once on the first successful load, + and disconnects so later reloads don't re-validate.""" + queue_parameters = [{"name": "CondaChannels", "type": "STRING"}] + _dialog, _widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + emit_after=[ + ("clear", []), + ("load", queue_parameters), + ("load", [{"name": "Other"}]), + ], + ) + + validate_mock.assert_called_once() + assert validate_mock.call_args.args[2] == queue_parameters + + def test_destroy_after_validation_does_not_warn( + self, qtbot, fresh_deadline_config, tmp_path, recwarn + ): + """Dialog destruction after validation already ran (and self-disconnected) must not + attempt a second disconnect — PySide6 reports that with a RuntimeWarning.""" + dialog, _widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + emit_after=[("load", [{"name": "CondaChannels"}])], + ) + validate_mock.assert_called_once() + + dialog.destroyed.emit() + + assert not [w for w in recwarn.list if "disconnect" in str(w.message)] + + def test_dialog_closed_before_load_disconnects_validator( + self, qtbot, fresh_deadline_config, tmp_path + ): + """Ordinarily closing the dialog (user hits X/Esc; no WA_DeleteOnClose, so the + QObject stays alive and ``destroyed`` never fires) before any successful load + tears down the connection: a later success emission from the long-lived + singleton must not run the validator against the closed dialog.""" + dialog, widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + ) + qtbot.addWidget(dialog) + + # Make the dialog visible (as show_job_bundle_submitter does for real) and + # close it the ordinary way — closeEvent -> reject() -> finished. + QDialog.show(dialog) + assert dialog.isVisible() + QDialog.close(dialog) + assert not dialog.isVisible() + + with patch( + "deadline.client.ui.job_bundle_submitter._validate_and_warn_about_parameters", + validate_mock, + ): + # A successful-load emission (e.g. triggered by another consumer of the + # singleton), which would run the validator if the connection were live. + widget._controller.queue_parameters_load_succeeded.emit([{"name": "CondaChannels"}]) + + validate_mock.assert_not_called() + + def test_dialog_destroyed_disconnects_validator(self, qtbot, fresh_deadline_config, tmp_path): + """Destroying the dialog before queue params load tears down the connection, so the + stale closure never fires against the singleton controller.""" + dialog, widget, validate_mock = self._run( + qtbot, + tmp_path, + job_parameters=[{"name": "Foo", "value": "bar"}], + validate_side_effect=lambda *a, **k: True, + ) + + # Simulate the dialog being destroyed before any load completes. + dialog.destroyed.emit() + + with patch( + "deadline.client.ui.job_bundle_submitter._validate_and_warn_about_parameters", + validate_mock, + ): + # A successful-load emission, which would run the validator if the + # connection were still live. + widget._controller.queue_parameters_load_succeeded.emit([{"name": "CondaChannels"}]) + + validate_mock.assert_not_called()