Ticket3071 add ioc kicker #121
Conversation
ghost
mentioned this pull request
README.md
Outdated
| @@ -97,6 +97,18 @@ To add a another suite of tests: | |||
| * Create a test class (deriving from `unittest.TestCase`) in your module and fill it with tests. This no longer has to have a specific name. You can have multiple test classes within a test module, all of them will be executed. | |||
| * Done! | |||
|
|
|||
| #### Adding a suite of tests as a Python module | |||
|
|
|||
There was a problem hiding this comment.
This is a good description of how to set up a test suite, but can you include information about what a test suite is and in which circumstances using a test suite is better and worse?
tests/kicker/test_basic_commands.py
Outdated
| @parameterized.expand( | ||
| parameterized_list([4.68, 6, 10, 0, 4e-3]) | ||
| ) | ||
| def test_that_GIVEN_a_value_THEN_the_calibrated_value_is_read(self, _, value_to_set): |
There was a problem hiding this comment.
The names of these tests are very vague, which I think is because of the multiple purposes each test needs to conform to as part of the test suite. As a result the meaning of each test is hidden and can make them hard to read, and it is more difficult to diagnose a failed test. Similar for other tests.
tests/kicker/test_basic_commands.py
Outdated
| self._simulate_value(0) | ||
|
|
||
| def _simulate_value(self, value): | ||
| pv_root = "DAQ:{}".format(self.record) |
There was a problem hiding this comment.
This method depends on the state that the class is in, which makes reading/debugging challenging. Could the data held in self.record be passed in as an argument instead?
tests/kicker/test_basic_commands.py
Outdated
|
|
||
| # Then: | ||
| self.ca.assert_that_pv_alarm_is(self.record, self.ca.Alarms.MAJOR) | ||
|
|
There was a problem hiding this comment.
If the waveform records cannot be tested, could there be a comment somewhere explaining why? Similar for the Schneider PLC, but I understand that can't be interfaced with yet.
tests/kicker/test_basic_commands.py
Outdated
| self.ca.assert_that_pv_is_number(self.record, expected_calibrated_value, 0.01) | ||
|
|
||
| @parameterized.expand( | ||
| parameterized_list([15, -5]) |
There was a problem hiding this comment.
Are the current and voltage records tested using the same values? This might have unintended consequences when testing against current/voltage limits.
Description of work done
Ticket
#3404