-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i13-1 merlin test initial commit #965
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huw-dls this looks good! I think it will work, I ran the system tests (dodal connect i13-1
) and the detector found the PVs okay. That command uses the same code as blueapi to pick up devices so that should also work.
The problem is that this code is not loaded into your beamline module at /dls_sw/i13-1/epics/scratch/dodal/src/dodal/beamlines/i13_1.py
, I would suggest we get this merged and then just checkout the main
branch in that location, so you have an up-to-date dodal and Merlin to play with. See the issues I've raised they are very minor.
Additional nit: Please remove the commented-out extra lines before merging.
@huw-dls I think the rebase has gone wrong somewhere, the diff shouldn't have all of those extraneous changes in it, what I normally do:
Then work through the conflicts until it is happy |
@huw-dls Happy to approve once CI passes |
@huw-dls quick note: I know it says "fixes" but that's just a Github shorthand for "please automatically close this issue/ticket when this is merged", we also use it for features. In general we try to write an issue for almost everything we do (can except quick patches and similar) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
=======================================
Coverage 97.48% 97.48%
=======================================
Files 148 151 +3
Lines 6238 6290 +52
=======================================
+ Hits 6081 6132 +51
- Misses 157 158 +1 ☔ View full report in Codecov by Sentry. |
@huw-dls This is looking good and almost done. The new devices need tests to keep the coverage happy. This is an important way to prevent the buildup of dead code. Have a look at tests for similar devices, they are small so shouldn't be too hard to test. |
…h was being set to 2022)
a4d6d0b
to
f9c28e6
Compare
Feature (not fix)
Instructions to reviewer on how to test:
merlin_controller will be finished after testing on i13-1.
The merlin device will then be moved from i13-1 to the top-level devices directory under merlin/.
Checks for reviewer
dodal connect ${BEAMLINE}