Add / Update Unit Tests#58
Conversation
16c119a to
515a425
Compare
82a6a92 to
14b20f5
Compare
d2140c3 to
a55c136
Compare
a55c136 to
9e7d00d
Compare
MJGaughran
left a comment
MJGaughran
left a comment
There was a problem hiding this comment.
I've added some initial feedback. I think a number apply to the majority of test cases.
I understand that this PR is still in progress, so just keep it as a Draft for now.
| machine = machine_setup | ||
| algorithm = TEST_ALG(machine) | ||
| single_offsets = algorithm.create_offsets_dict(TEST_RESULTS_SINGLE, TEST_METADATA) | ||
| algorithm.apply_bba_offsets(single_offsets) |
There was a problem hiding this comment.
Is it possible to check whether the offsets are actually getting applied? e.g.
assert bpm.offset == new_value
but using the actual attributes.
There was a problem hiding this comment.
Not with the current structure of the unit tests.
| @mock.patch("dls_bba.machine.Machine.apply_feedbacks", return_value=None) | ||
| @mock.patch("dls_bba.worker.ask_question", side_effect=[True]) | ||
| @mock.patch("dls_bba.algorithm.bba_offsets_plot", return_value=None) | ||
| def test_use_bba_offsets_yes_pass( |
There was a problem hiding this comment.
As above, there is no difference in the test for either True or False responses so you are not testing the actual outcome. A unit test can also query a mock object to ensure it has been interacted with correctly. Happy to talk about this as it isn't always easy.
|
|
||
| @mock.patch("dls_bba.machine.Machine.get_beam_current", side_effect=[0]) | ||
| @mock.patch("dls_bba.worker.ask_question", side_effect=[None]) | ||
| def test_beamcurrentcheck_init(mock_ask_question, mock_beam_current, machine_setup): |
There was a problem hiding this comment.
I'm not sure testing basic init methods is very helpful. I think this is made more obvious by the fact that you have to query a protected (sort of private) attribute.
| @mock.patch("dls_bba.machine.Machine.get_beam_current", side_effect=[0, 16, 17]) | ||
| @mock.patch("dls_bba.worker.ask_question", side_effect=[True]) | ||
| @mock.patch("dls_bba.machine.Machine.check_feedbacks", return_value=None) | ||
| def test_beamcurrentcheck_topup_user_returns_yes( |
There was a problem hiding this comment.
This is relevant to many tests. There are several function naming issues:
- Repeating the name of the class. This is already specified in the test file name.
- Not including conjunctions like if, when etc
- Shortening / reducing method names for little gain
test_topup_beam_runs_topup_when_user_selects_yes
| @mock.patch("dls_bba.machine.Machine.get_beam_current", side_effect=[0, 14, 16]) | ||
| @mock.patch("dls_bba.worker.ask_question", side_effect=[True, True]) | ||
| @mock.patch("dls_bba.machine.Machine.check_feedbacks", return_value=None) | ||
| def test_beamcurrentcheck_topup_user_returns_yes_then_yes_then_pass( |
There was a problem hiding this comment.
The previous test suggests everything works fine if the user selects Yes. But then we have the additional Yes and Pass.
Something is missing from this test name. Same for the next test.
| "dls_bba.machine.Machine._get_effective_corrector", | ||
| side_effect=_get_effective_corrector, | ||
| ) | ||
| def test_apply_golden_without_machine( |
There was a problem hiding this comment.
I've mentioned similar issues elsewhere; we need to ensure that the golden offsets are actually getting applied somewhere. This could be as simple as asserting that the correct (public) function is getting called, or checking e.g. one of several PVs and values set by caput has the correct form.
| def machine_setup(): | ||
| machine = Machine() | ||
| return machine | ||
| def test_from_name_h(machine_setup): |
There was a problem hiding this comment.
Use more complete sentences and clear abbreviations.
There was a problem hiding this comment.
i.e. it doesn't have to be a complete sentence! Just more than this
| machine.quad2bpm("INVALID_QUADRUPOLE") | ||
|
|
||
|
|
||
| # TODO: Test get_enabled_bpms / measure_bpms / get_bba_offsets / retry_command. |
There was a problem hiding this comment.
Do you intend to add these for this PR?
ee7c0d7 to
102ca27
Compare
JoshuaAppleby
added
python
2 participants
Add comprehensive unit tests and add proper typing for modules such as GUI.