test GUI file for GUI ca exp creator - #219
Conversation
ludivintchokote
commented
Jan 30, 2025
ludivintchokote
commented
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
this is good progress...these PRs are clean now! Please see my inline comments.
| @@ -0,0 +1,23 @@ | |||
| **Added:** | |||
|
|
|||
| * Added pytest code for gui_ca_exp_creator | |||
There was a problem hiding this comment.
This is really great, thanks for doing this! Since this will go into the changelog at the next release, we only need a single statement that we added tests for the GUI code. To make the code pass pre-commit CI please put this statement in each news file:
No news needed: one part of a larger set of tests being added for gui code
except for one news file where we want to write:
Extensive tests for GUI code
under Added
|
|
||
|
|
||
| # Mocking the UI class | ||
| class MockUiCA: |
There was a problem hiding this comment.
do we want this in the conftest.py file as a fixture as we will reuse it in other tests?
| def __init__(self): | ||
| self.experiment_file_path = MagicMock() | ||
| self.load_folder_name = MagicMock(return_value=tempfile.gettempdir()) | ||
| self.AP_window = MagicMock(return_value=(MagicMock(), MagicMock())) |
There was a problem hiding this comment.
to keep code consistent with other projects (and therefore more easily maintainable) is it possible to use pytest-mock instead of MagicMock? I think you may be able to find some examples in the diffpy.utils code. If not, I can dig out some examples.
| self.save_experiment_file_button = MagicMock() | ||
|
|
||
|
|
||
| class MockUiExperiment: |
There was a problem hiding this comment.
same here, think about putting in conftest.py as a fixture? Not sure about this, so please push back if you think these are bad ideas.....
| time_converter, | ||
| ) | ||
|
|
||
| # Sample data for testing |
There was a problem hiding this comment.
Let's remove comments where they contain essentially no information
| } | ||
|
|
||
|
|
||
| # Mocking the UI class |
| self.experiment_time_step = MagicMock() | ||
|
|
||
|
|
||
| # Test for load_folder function |
There was a problem hiding this comment.
remove comment, I can see this information from the following line.
However, missing is a comment that describes what the intended behavior is. In general we spend quite a bit of time to understand the intended behavior before writing a test and we use comments to do that. In the case of these GUI tests, I think it is less important as the code is already written and we are just trying to back-fill. But at least having some idea about what behavior we want the function to have will help us understand our code better.
As an example, here are testing load_folder(). What do we want to happen under different circumstances, i.e. when it gets a valid input, when a folder is missing, etc.?
|
|
||
| # Test for get_AP_parameters function | ||
| def test_get_AP_parameters(): | ||
| exp = MockUiExperiment() |
There was a problem hiding this comment.
I am a little confused about this....are we actually testing anything in the code or just testing the mock class that you defined above?
|
@q24609962-svg at our hackathon last year effort was put into writing tests, but for various reasons they didn't get finished and merged. Tests are always good, so it would be a great service if you could see whether you could clean these up and get them merged, make them better if you can and even write more tests if you are inclined. At least getting a few more tests merged so as we fix things and add features there is less chance that we break the code. In other words, doing the tests before the fixes is better than doing the tests after the fixes... |
3 participants
