Skip to content
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

Add a method on the channel parameter to check if it is closed. #1622

Merged
merged 6 commits into from
Jul 8, 2019

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Jul 2, 2019

Currently, to check if a channel on the Keithley S46 is closed we have to write the following:

s46.A1.get() == "close"

This can be error prone: typos can cause this code to fail (e.g. 'closed' instead of 'close', 'Close' with a capital C, accidental spaces, etc).

Furthermore, a bug of this nature can go unnoticed for a long time and might be hard to track down, as the code above will not generate an exception when 'close' is spelled wrong.

The following code is much more robust

s46.A1.is_closed()

An understandable error message will be thrown when the above code is run with a typo in the method name.

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Shall the example notebook be updated as well?

and what an unfortunate thing that "closed" state of the channel is called "close" (without "d") :)

qcodes/instrument_drivers/tektronix/Keithley_s46.py Outdated Show resolved Hide resolved
sochatoo added 2 commits July 2, 2019 14:31
…ations: `test_is_closed` should be run before opening/closing channels on the mock instrument.
@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #1622 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1622   +/-   ##
=======================================
  Coverage   70.55%   70.55%           
=======================================
  Files         123      123           
  Lines       14921    14921           
=======================================
  Hits        10528    10528           
  Misses       4393     4393

@astafan8
Copy link
Contributor

astafan8 commented Jul 3, 2019

carefully determine the order of tests to get around pyvisa-sim limit… …
…ations: test_is_closed should be run before opening/closing channels on the mock instrument.

please do not depend on the order the tests are executed. this is so annoying to debug if there are problems (and in fact we recently had such an experience). just prepare the insturment in the correct state in the test (or in a separate fixture), and then test the method.

@astafan8 astafan8 added the driver label Jul 3, 2019
sochatoo added 2 commits July 5, 2019 10:00
…d "close" with lower case string, while the yaml file contains handlers for upper case.

This causes the mock instrument to return "ERROR" after the test 'test_locking_mechanism'. This has gone unnoticed for a long time because there were no test after 'test_locking mechanism'.

@mikhail Astafev: You were right mate, to call me out on this. It caused me to find a bug :-)
@sohailc
Copy link
Member Author

sohailc commented Jul 8, 2019

@astafan8 can you have another look at this, please?

@astafan8 astafan8 closed this Jul 8, 2019
@astafan8 astafan8 reopened this Jul 8, 2019
@astafan8 astafan8 merged commit cb47310 into microsoft:master Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants