Training rigs system test #764
Conversation
ZohebShaikh
force-pushed
the
training-rigs-system-test
branch
from
d71d3e7 to
dcb076c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #764 +/- ##
=======================================
Coverage 92.92% 92.92%
=======================================
Files 37 37
Lines 2063 2063
=======================================
Hits 1917 1917
Misses 146 146 ☔ View full report in Codecov by Sentry. |
ZohebShaikh
force-pushed
the
training-rigs-system-test
branch
from
dcb076c to
7b0245f
Compare
ZohebShaikh
force-pushed
the
training-rigs-system-test
branch
from
647d489 to
8f8f72d
Compare
|
As discussed, I think we should make a new repository for our beamline system tests. I can make a new one and review these changes there or review them here, which is easier @ZohebShaikh? Either way I'm going to put in bold that... THIS PR SHOULD NOT BE MERGED! |
|
@callumforrester We can review the changes here. In my opinion, it would be a good idea to create a new repository in GitLab. This way, we can store sensitive information, such as the cluster IP address, in plain text without concerns. It might be best if you could create the repository, and I’ll start migrating the files once it’s ready. |
|
Okay, I'll have a think about where to put it. The matter your raise actually came up in the DAQ away day earlier and we concluded (with security people present) that a lot of information like that is not actually considered sensitive. |
| @@ -338,6 +260,30 @@ def test_get_current_state_of_environment(client: BlueapiClient): | |||
| assert client.get_environment() == EnvironmentResponse(initialized=True) | |||
|
|
|||
|
|
|||
| @pytest.mark.xfail( | |||
There was a problem hiding this comment.
Must: This test should be skipped rather than xafiled, xfail is for tests that fail all the time, not intermittently. It just has the effect of reversing the result (i.e. if the test sometimes passes the suite will fail)
There was a problem hiding this comment.
If this is moved to a new repository, the test_blueapi_system.py file will no longer be present, so the issue won't arise. I can add the back-off logic to the current system test. The reason it’s passing in the main branch is that it pre-emptively receives the response for the initialized environment. Alternatively, we could simply add a skip.
There was a problem hiding this comment.
Worth noting I have a long term solution WIP here: #743
|
|
||
|
|
||
| def check_all_events(all_events: list[AnyEvent]): | ||
| assert isinstance(all_events[0], WorkerEvent) and all_events[0].task_status |
There was a problem hiding this comment.
Should: Be explicit
| assert isinstance(all_events[0], WorkerEvent) and all_events[0].task_status | |
| assert isinstance(all_events[0], WorkerEvent) and (all_events[0].task_status is not None) |
There was a problem hiding this comment.
I think this approach is good for the general system tests that run in CI. However for beamlines it is probably too restrictive, we don't want to have to change this file for every change we make to any plan. Instead the tests should identify several critical capabilities and test that they are present e.g.
"is there a plan called X?"
"does Y happen when I run it?"
"is there a device called Z?"
You also shouldn't have to worry to about testing for the very basic capabilities of blueapi (e.g. can I reload the environment?).
There was a problem hiding this comment.
Test which does this is here : https://github.com/DiamondLightSource/blueapi/blob/training-rigs-system-test/tests/system_tests/test_plans.py
But I agree I can wait till a place for it is decided and then I will move this test in that
|
@ZohebShaikh To expand on the Gitlab conversation, I think I'm settling on Github. The premise seems to be that there are 3 categories of information: Not sensitive at all, internally sensitive and totally sensitive and that internally sensitive information is fine to store under VCS as long as its not on the public internet, as opposed to totally sensitive information like passwords, which should never be stored (unencrypted) under VCS under any circumstances. I'm going to posit that the actual amount of information that is internally sensitive is small, if not zero. Meanwhile if go on Gitlab then we lose a lot of the free infrastructure that we get with the copier template (CI/CD, issue template etc.) |
|
@callumforrester I’m happy with moving forward with GitHub |
|
Closing PR will add tests to the new repo |
No description provided.