-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adjust testing environment #23
Conversation
d190f0a
to
164f08b
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #23 +/- ##
==========================================
- Coverage 83.48% 83.34% -0.14%
==========================================
Files 7 7
Lines 1072 1075 +3
==========================================
+ Hits 895 896 +1
- Misses 177 179 +2
☔ View full report in Codecov by Sentry. |
61654cf
to
2c5dee1
Compare
Dist is also fails on main and dev |
13c58a1
to
77f2837
Compare
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.
I like the overall structural changes to the tests - it's much nicer to not have the dummy_server
, and its associated process and network calls. It should be much easier going forward to reason about how the tests will run.
I'm requesting changes as I think important tests have been removed in this refactoring. None of the tests that checked the dummy server have received expected messages have been kept, which means we have no verification that this module is sending any of the expected messages to PandA. Given our talks about Mocks I expected to see some mocking of the PandA-client
calls and validating that they have received the messages that we previously saw in the dummy server.
I've now passed a |
f8c0aca
to
d1917d8
Compare
pyproject.toml
Outdated
@@ -19,7 +19,7 @@ dependencies = [ | |||
"h5py", | |||
"softioc>=4.3.0", | |||
"pandablocks>=0.3.1", | |||
"pvi[cli]>=0.4", | |||
"pvi[cli]<0.5", |
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.
pvi requires 3.10 for v0.5, it's neccessary to pin this for consistency on .bob
files between 3.9 and 3.10/3.11.
Maybe we should consider requiring 3.10 for pandablocks-ioc @AlexanderWells-diamond?
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.
#26 this will require pvi>=0.5
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.
I don't know why PVI updated itself to Python 3.10 - it appears to be this PR that did it, but there are no functional changes to the Python code that warrant dropping lower Python versions - the only changes appear to be in the typing
module, which will not affect runtime behaviour.
Ultimately I don't really mind what version requirements we have. I'll defer to whatever @coretl and @GDYendell want to do.
30c3bcb
to
06709f1
Compare
…d the name of DummyServer to MockedServer
…e working callstack
… retrieving commands from the to give them time
…nd 3.10/3.11. We should consider 3.10 minimum on pandablocks-ioc
… give a deprecation warning on python 3.10 if you don't
06709f1
to
a9139d8
Compare
…nstall requires). Should hopefully fix the problem of deprecation warnings on python/3.8
f06151d
to
f8fa658
Compare
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.
Looks good now. I've just raised a couple more very minor comments, but they don't have to be resolved to merge this PR.
4c47b82
to
4e7e53a
Compare
to use waits, changed the mocked_panda_standard_responses to wait until the MockedAsyncioClient is set up before proceeding with the test.
4e7e53a
to
9c88dae
Compare
to allow bobfiles time to be written in the subprocess. Changed to pvi>=0.5 and python>=3.10
4771eb6
to
e289cb3
Compare
No description provided.