pyvisa / pyvisa-sim

A PyVISA backend that simulates a large part of the "Virtual Instrument Software Architecture" (VISA_)
https://pyvisa-sim.readthedocs.io/en/latest/
MIT License
70 stars 39 forks source link

Bugfixes #23

Closed famish99 closed 9 years ago

famish99 commented 9 years ago

Fixed unexpected read timeout behavior. Fixed loading of config files that aren't the default config.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+1.95%) to 81.17% when pulling 988b872c6048f2e2e90aa1741b2d5e96c1647f94 on famish99:master into 01bd79f8fa235a3e66ea2a3cbcc76898493437b3 on hgrecco:master.

hgrecco commented 9 years ago

Can you provide some arguments for this commit. I think the current behavior is the correct one. If nothing is read, it should try the loop again (continue) without going through the whole loop.

famish99 commented 9 years ago

It will not time out because "now" is not being reread. On Mar 26, 2015 8:30 PM, "Hernan Grecco" notifications@github.com wrote:

Can you provide some arguments for this commit. I think the current behavior is the correct one. If nothing is read, it should try the loop again (continue) without going through the whole loop.

— Reply to this email directly or view it on GitHub https://github.com/hgrecco/pyvisa-sim/pull/23#issuecomment-86784610.

famish99 commented 9 years ago

You could avoid the variable altogether to continue to use the continue, I can make the change later, just didn't occur to me at the time On Mar 26, 2015 9:34 PM, "Huan Nguyen" famish99@gmail.com wrote:

It will not time out because "now" is not being reread. On Mar 26, 2015 8:30 PM, "Hernan Grecco" notifications@github.com wrote:

Can you provide some arguments for this commit. I think the current behavior is the correct one. If nothing is read, it should try the loop again (continue) without going through the whole loop.

— Reply to this email directly or view it on GitHub https://github.com/hgrecco/pyvisa-sim/pull/23#issuecomment-86784610.

hgrecco commented 9 years ago

You are right, now = ... ended up in the wrong place. But I don't like the idea of flowing through the whole while. I think the right way would be to remove that variable and just do time.time() - start in the while condition.

Another question: You also added a test with unexpected read. There is no such thing as that error. reading when there is nothing to read yields a timeout.

famish99 commented 9 years ago

Yeah it should expect an exception. It's just what I call the test to know where it failed. On Mar 27, 2015 6:56 AM, "Hernan Grecco" notifications@github.com wrote:

You are right, now = ... ended up in the wrong place. But I don't like the idea of flowing through the whole while. I think the right way would be to remove that variable and just do time.time() - start in the while condition.

Another question: You also added a test with unexpected read. There is no such thing as that error. reading when there is nothing to read yields a timeout.

— Reply to this email directly or view it on GitHub https://github.com/hgrecco/pyvisa-sim/pull/23#issuecomment-86912765.

famish99 commented 9 years ago

I will update the fix when I get to work, just want to make sure it passes all my tests before checking it in. On Mar 27, 2015 6:56 AM, "Hernan Grecco" notifications@github.com wrote:

You are right, now = ... ended up in the wrong place. But I don't like the idea of flowing through the whole while. I think the right way would be to remove that variable and just do time.time() - start in the while condition.

Another question: You also added a test with unexpected read. There is no such thing as that error. reading when there is nothing to read yields a timeout.

— Reply to this email directly or view it on GitHub https://github.com/hgrecco/pyvisa-sim/pull/23#issuecomment-86912765.

hgrecco commented 9 years ago

Great!

famish99 commented 9 years ago

Regarding the "unexpected read" test, I'd be preferred if it was left in but maybe renamed to "Read empty buffer" because my internal unit tests require the behavior.

Perhaps somewhere in the resource initialization, the timeout can be shorted to improve test time, but otherwise I'd rather keep the test in to ensure it won't break again in the future.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+1.56%) to 80.78% when pulling 965a189768f5fc394b9c40ac17242874541901e5 on famish99:master into 01bd79f8fa235a3e66ea2a3cbcc76898493437b3 on hgrecco:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+1.95%) to 81.17% when pulling 965a189768f5fc394b9c40ac17242874541901e5 on famish99:master into 01bd79f8fa235a3e66ea2a3cbcc76898493437b3 on hgrecco:master.

hgrecco commented 9 years ago

Sorry if I was unclear. It is fine to keep the test and is also fine that we raise a VisaIOError(constants.VI_ERROR_TMO). What I do not want is to diverge from pyvisa.

famish99 commented 9 years ago

Yeah, I think the test is consistent with pyvisa, but the naming sense is probably off because I just copied it from my internal unit tests.

famish99 commented 9 years ago

Do you want to merge this pull request before I add another bugfix or should I combine it with this pull request?

hgrecco commented 9 years ago

I will merge them on friday. Thanks!

famish99 commented 9 years ago

Is this even merge-able anymore?

famish99 commented 9 years ago

This should be a pretty minor pull request.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.89%) to 83.91% when pulling 76b08cd4de02141fcdfd06ca2c56da5c83dea47c on famish99:master into 8f6d0a16489ab959b04da8609a894d5025ec8883 on hgrecco:master.