From 62441b9bbd21e9ac30afa2544ee86828e1fd3712 Mon Sep 17 00:00:00 2001 From: Marie Backman Date: Mon, 27 Nov 2023 12:44:57 -0500 Subject: [PATCH 1/2] Fix defect: RefRed crashes when pressing Enter in an empty cell in the reduction table --- RefRed/main.py | 2 +- .../update_reduction_table.py | 11 +++--- .../test_update_reduction_table.py | 34 +++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/RefRed/main.py b/RefRed/main.py index 4a4d235d..790206cf 100644 --- a/RefRed/main.py +++ b/RefRed/main.py @@ -52,7 +52,7 @@ class MainGui(QtWidgets.QMainWindow): '''Top class that handles the GUI''' - file_loaded_signal = QtCore.Signal() + file_loaded_signal = QtCore.Signal(int, bool, bool) # default location path_ascii = '.' # ascii file such as scaling factor file diff --git a/RefRed/reduction_table_handling/update_reduction_table.py b/RefRed/reduction_table_handling/update_reduction_table.py index 3365cf03..804c58ef 100644 --- a/RefRed/reduction_table_handling/update_reduction_table.py +++ b/RefRed/reduction_table_handling/update_reduction_table.py @@ -19,16 +19,16 @@ def __init__(self, parent=None, row=0, col=1, runs=None): self.row = row self.col = col + data_type = 'data' if col == 1 else 'norm' + self.is_data_displayed = True if (col == 1) else False + item = self.parent.ui.reductionTable.item(row, col) if item.text() == '': self.clear_cell(row, col) - self.parent.file_loaded_signal.emit() + self.parent.file_loaded_signal.emit(row, self.is_data_displayed, self.display_of_this_row_checked()) QApplication.processEvents() return - data_type = 'data' if col == 1 else 'norm' - self.is_data_displayed = True if (col == 1) else False - self.raw_runs = str(runs) run_breaker = RunSequenceBreaker(run_sequence=self.raw_runs) list_run = run_breaker.final_list @@ -85,6 +85,9 @@ def clear_cell(self, row, col): # Note: there is a Mantid process cleaning process elsewhere in the code # so we don't have to deal with it here. config = self.parent.big_table_data[self.row, 2] + if config is None: + # no data loaded in this row + return if col == 1: self.parent.big_table_data[self.row, 0] = None config.clear_data() diff --git a/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py b/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py index 8b64a849..2c9210f8 100644 --- a/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py +++ b/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py @@ -83,3 +83,37 @@ def test_update_reduction_table_thread( # check that the reduction table was re-enabled after the thread returned assert window_main.ui.reductionTable.isEnabled() + + +@pytest.mark.parametrize("column", [1, 2]) # data run column, normalization run column +@mock.patch("RefRed.reduction_table_handling.update_reduction_table.LocateListRun") +def test_update_reduction_table_clear_cell(mock_locate_list_run, data_server, qtbot, column): + """Test pressing Enter in empty cell in the reduction table""" + mock_locate_list_run.return_value = MockLocateListRun() + + window_main = MainGui() + qtbot.addWidget(window_main) + + # test pressing Enter in empty cell before any data has been loaded + window_main.ui.reductionTable.setCurrentCell(0, column) + window_main.ui.table_reduction_cell_enter_pressed() + qtbot.wait(wait) + # check that the reduction table was re-enabled after the function returned + assert window_main.ui.reductionTable.isEnabled() + + # test first loading a run, then clearing the cell and pressing Enter to clear loaded data + window_main.ui.reductionTable.setCurrentCell(0, column) + window_main.ui.reductionTable.currentItem().setText("184975") + window_main.ui.table_reduction_cell_enter_pressed() + qtbot.wait(5000) + # check that the data has been loaded + assert window_main.big_table_data[0, column - 1] is not None + # clear the cell and press Enter to clear the data + window_main.ui.reductionTable.setCurrentCell(0, column) + window_main.ui.reductionTable.currentItem().setText("") + window_main.ui.table_reduction_cell_enter_pressed() + qtbot.wait(wait) + # check that the reduction table was re-enabled after the function returned + assert window_main.ui.reductionTable.isEnabled() + # check that the data has been cleared + assert window_main.big_table_data[0, column - 1] is None From 1f49a2a7ccfc2ce2999ef452210d13a69d8fbf59 Mon Sep 17 00:00:00 2001 From: Marie Backman Date: Fri, 1 Dec 2023 12:31:18 -0500 Subject: [PATCH 2/2] when loading a run, don't update the peak and background ranges if the same run is being reloaded --- .../check_list_run_compatibility_thread.py | 16 ++ .../reduction_table_check_box.py | 3 + development.yml | 7 +- test/conftest.py | 18 +++ .../test_update_reduction_table.py | 147 +++++++++--------- 5 files changed, 112 insertions(+), 79 deletions(-) diff --git a/RefRed/calculations/check_list_run_compatibility_thread.py b/RefRed/calculations/check_list_run_compatibility_thread.py index 2657fb42..c02f56e8 100644 --- a/RefRed/calculations/check_list_run_compatibility_thread.py +++ b/RefRed/calculations/check_list_run_compatibility_thread.py @@ -107,10 +107,26 @@ def update_lconfigdataset(self): self.parent.big_table_data = big_table_data def loading_lr_data(self): + """Updates data table with the new run.""" wks = self.wks lrdata = LRData(wks, parent=self.parent) self.lrdata = lrdata big_table_data = self.parent.big_table_data col_index = 0 if self.is_working_with_data_column else 1 + # keep existing user config if the new run number is the same as the old one + if self.is_reloading_same_run(): + lrdata.peak = big_table_data[self.row, col_index].peak + lrdata.back = big_table_data[self.row, col_index].back + lrdata.low_res = big_table_data[self.row, col_index].low_res big_table_data[self.row, col_index] = lrdata self.parent.big_table_data = big_table_data + + def is_reloading_same_run(self): + """Checks if the new run number is the same as the old, i.e. the run is being reloaded.""" + col_index = 0 if self.is_working_with_data_column else 1 + big_table_data_run = self.parent.big_table_data[self.row, col_index] + if big_table_data_run is not None and ( + big_table_data_run.run_number == self.parent.ui.reductionTable.item(self.row, self.col).text() + ): + return True + return False diff --git a/RefRed/reduction_table_handling/reduction_table_check_box.py b/RefRed/reduction_table_handling/reduction_table_check_box.py index f80949c1..4cad5424 100644 --- a/RefRed/reduction_table_handling/reduction_table_check_box.py +++ b/RefRed/reduction_table_handling/reduction_table_check_box.py @@ -57,6 +57,9 @@ def update_gui(self): if self.is_row_selected_checked(_row_selected): _big_table_data = self.parent.big_table_data _lconfig = _big_table_data[_row_selected, 2] + if not _lconfig: + # no data loaded on this row + return if bool(_lconfig.tof_auto_flag): self.parent.ui.dataTOFautoMode.setChecked(True) else: diff --git a/development.yml b/development.yml index b0b15605..12f51f89 100644 --- a/development.yml +++ b/development.yml @@ -1,12 +1,13 @@ name: refred channels: - - mantid + - mantid/label/main - conda-forge - defaults dependencies: - - mantid==6.4 + - python=3.8 + - mantidworkbench>=6.7.0 - qt>=5.12.9,<6 - - pyqt>=5.12.3,<5.15 + - pyqt>=5.12.3 - qtpy>=1.9.0 - pip - configobj diff --git a/test/conftest.py b/test/conftest.py index 17e5590b..e3e8525a 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -113,3 +113,21 @@ def handler(): return window return _main_gui + + +@pytest.fixture +def file_finder_find_runs(data_server): + """Fixture to add as side effect for mock of FileFinder.findRuns to locate runs in the test data directory + + Usage: + @mock.patch("RefRed.calculations.locate_list_run.FileFinder.findRuns") + def test_update_reduction_table(mock_file_finder_find_runs, file_finder_find_runs): + mock_file_finder_find_runs.side_effect = file_finder_find_runs + :return function: function to add as side effect to mock of FileFinder.findRuns + """ + + def _file_finder_find_runs(file_hint: str): + """Get path to file in test data directory""" + return data_server.path_to(f"{file_hint}.nxs.h5") + + return _file_finder_find_runs diff --git a/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py b/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py index 2c9210f8..7adda4a0 100644 --- a/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py +++ b/test/unit/RefRed/reduction_table_handling/test_update_reduction_table.py @@ -2,80 +2,34 @@ # third party packages import pytest -from qtpy.QtCore import Qt import unittest.mock as mock -wait = 200 +wait = 1000 -class Event(object): - val = None - - def __init__(self, val=None): - self.val = val - - def key(self): - return self.val - - -class MockLocateListRun(object): - """Mock return value for LocateListRun""" - - list_run = [] - list_nexus_found = ['/SNS/REF_L/IPTS-26776/nexus/REF_L_184975.nxs.h5'] - list_run_found = [184975] - list_run_not_found = [] +def load_run_from_reduction_table(window_main, row: int, col: int, run: str): + """Add run number in reduction table cell and press Enter to load a run""" + window_main.ui.reductionTable.setCurrentCell(row, col) + window_main.ui.reductionTable.currentItem().setText(run) + window_main.ui.table_reduction_cell_enter_pressed() @pytest.mark.parametrize("is_display_checked", [True, False]) -@mock.patch( - "RefRed.calculations.check_list_run_compatibility_thread.CheckListRunCompatibilityThread.updating_reductionTable_metadata" # noqa E501 -) -@mock.patch("RefRed.calculations.check_list_run_compatibility_thread.CheckListRunCompatibilityThread.loading_lr_data") -@mock.patch( - "RefRed.calculations.check_list_run_compatibility_thread.CheckListRunCompatibilityThread.update_lconfigdataset" -) -@mock.patch("RefRed.calculations.check_list_run_compatibility_thread.AddListNexus") -@mock.patch("RefRed.main.ReductionTableCheckBox") @mock.patch("RefRed.main.DisplayPlots") -@mock.patch("RefRed.reduction_table_handling.update_reduction_table.LocateListRun") +@mock.patch("RefRed.calculations.locate_list_run.FileFinder.findRuns") def test_update_reduction_table_thread( - mock_locate_list_run, - mock_display_plots, - mock_reduction_table_checkbox, - mock_add_list_nexus, - mock_update_lconfigdataset, - mock_loading_lr_data, - mock_updating_reductionTable_metadata, - is_display_checked, - qtbot, + mock_file_finder_find_runs, mock_display_plots, is_display_checked, qtbot, file_finder_find_runs ): - """Test of the communication between the main thread and CheckListRunCompatibilityThread - - Note: This only tests the signalling between the main thread and the thread spawned by - CheckListRunCompatibilityThread. The actual logic updating run data and plots is mocked. - """ - mock_locate_list_run.return_value = MockLocateListRun() - mock_add_list_nexus.return_value = mock.Mock(ws=True) + """Test of the communication between the main thread and CheckListRunCompatibilityThread""" + mock_file_finder_find_runs.side_effect = file_finder_find_runs window_main = MainGui() qtbot.addWidget(window_main) - # set the display checkbox to the desired value - window_main.ui.reductionTable.cellWidget(0, 0).setChecked(is_display_checked) - # set a run number in the reduction table - window_main.ui.reductionTable.setCurrentCell(0, 1) - window_main.ui.reductionTable.currentItem().setText("184975") - # press Enter in run number cell to trigger update_reduction_table - window_main.ui.reductionTable.keyPressEvent(Event(Qt.Key_Return)) + window_main.ui.reductionTable.cellWidget(0, 0).setChecked(is_display_checked) + load_run_from_reduction_table(window_main, 0, 1, "184975") qtbot.wait(wait) - # check mocked functions in the spawned thread - mock_update_lconfigdataset.assert_called_once() - mock_loading_lr_data.assert_called_once() - mock_updating_reductionTable_metadata.assert_called_once() - - # check that display plots is only called if the checkbox is checked if is_display_checked: mock_display_plots.assert_called_once() else: @@ -85,35 +39,76 @@ def test_update_reduction_table_thread( assert window_main.ui.reductionTable.isEnabled() -@pytest.mark.parametrize("column", [1, 2]) # data run column, normalization run column -@mock.patch("RefRed.reduction_table_handling.update_reduction_table.LocateListRun") -def test_update_reduction_table_clear_cell(mock_locate_list_run, data_server, qtbot, column): - """Test pressing Enter in empty cell in the reduction table""" - mock_locate_list_run.return_value = MockLocateListRun() +@pytest.mark.parametrize("col", [1, 2]) # data run column, normalization run column +@mock.patch("RefRed.calculations.locate_list_run.FileFinder.findRuns") +def test_empty_cell_press_enter(mock_file_finder_find_runs, col, qtbot, file_finder_find_runs): + """Test pressing Enter in empty cell in the reduction table before and after data has been loaded""" + mock_file_finder_find_runs.side_effect = file_finder_find_runs window_main = MainGui() qtbot.addWidget(window_main) # test pressing Enter in empty cell before any data has been loaded - window_main.ui.reductionTable.setCurrentCell(0, column) - window_main.ui.table_reduction_cell_enter_pressed() + load_run_from_reduction_table(window_main, 0, col, "") qtbot.wait(wait) # check that the reduction table was re-enabled after the function returned assert window_main.ui.reductionTable.isEnabled() # test first loading a run, then clearing the cell and pressing Enter to clear loaded data - window_main.ui.reductionTable.setCurrentCell(0, column) - window_main.ui.reductionTable.currentItem().setText("184975") - window_main.ui.table_reduction_cell_enter_pressed() - qtbot.wait(5000) + load_run_from_reduction_table(window_main, 0, col, "184975") + qtbot.wait(wait) # check that the data has been loaded - assert window_main.big_table_data[0, column - 1] is not None + assert window_main.big_table_data[0, col - 1] is not None # clear the cell and press Enter to clear the data - window_main.ui.reductionTable.setCurrentCell(0, column) - window_main.ui.reductionTable.currentItem().setText("") - window_main.ui.table_reduction_cell_enter_pressed() + load_run_from_reduction_table(window_main, 0, col, "") qtbot.wait(wait) - # check that the reduction table was re-enabled after the function returned - assert window_main.ui.reductionTable.isEnabled() # check that the data has been cleared - assert window_main.big_table_data[0, column - 1] is None + assert window_main.big_table_data[0, col - 1] is None + assert window_main.ui.reductionTable.isEnabled() + + +@mock.patch("RefRed.main.DisplayPlots") +@mock.patch("RefRed.calculations.locate_list_run.FileFinder.findRuns") +def test_load_run_auto_peak_finder(mock_file_finder_find_runs, mock_display_plots, qtbot, file_finder_find_runs): + """Test that the auto-peak finder is only enabled if a new run is loaded""" + mock_file_finder_find_runs.side_effect = file_finder_find_runs + + window_main = MainGui() + qtbot.addWidget(window_main) + + run1 = "188300" + expected_peak_run1 = ["130", "141"] + expected_back_run1 = ["127", "144"] + run2 = "188301" + expected_peak_run2 = ["130", "139"] + expected_back_run2 = ["127", "142"] + + # load the first run + load_run_from_reduction_table(window_main, row=0, col=1, run=run1) + qtbot.wait(wait) + # check the result of the auto-peak finder + assert window_main.big_table_data[0, 0].peak == expected_peak_run1 + assert window_main.big_table_data[0, 0].back == expected_back_run1 + + # modify the peak and background ranges (users can change these in the UI) + user_set_peak = ["132", "143"] + user_set_back = ["128", "147"] + window_main.big_table_data[0, 0].peak = user_set_peak + window_main.big_table_data[0, 0].back = user_set_back + # reload the same run + load_run_from_reduction_table(window_main, row=0, col=1, run=run1) + qtbot.wait(wait) + # check that the auto-peak finder did not change the peak and background ranges + assert window_main.big_table_data[0, 0].peak == user_set_peak + assert window_main.big_table_data[0, 0].back == user_set_back + + # load a different run in the cell + load_run_from_reduction_table(window_main, row=0, col=1, run=run2) + qtbot.wait(wait) + # check that the peak and background ranges were updated + assert window_main.big_table_data[0, 0].peak == expected_peak_run2 + assert window_main.big_table_data[0, 0].back == expected_back_run2 + + +if __name__ == '__main__': + pytest.main([__file__])