replace the call to serial.Serial with serial.serial_for_url #386

LongnoseRob · 2023-07-20T13:41:24Z

to support loop:// and other url-types supported in pyserial (>3.0)

tested with the follwing code on python 3.11.3, pyserial 3.5:

import pyvisa
rm = pyvisa.ResourceManager("@py")
abc = rm.open_resource("ASRLloop://::INSTR")
abc.timeout = 3000
abc.read_termination = "\r"
abc.write_termination = "\r\n"
abc.write("Test1234567890")
print(abc.read())

Closes [COM] Communication issue loop #385
Executed black . && isort -c . && flake8 with no errors
The change is fully covered by automated unit tests
Documented in docs/ as appropriate
Added an entry to the CHANGES file

to support loop:// and other url-types supported in pyserial (>3.0)

MatthieuDartiailh · 2023-07-20T20:07:57Z

Could add an entry to the docs ? Otherwise this looks good since we require pyserial >= 3.0 and this feature was introduced in 2.5

LongnoseRob · 2023-07-21T13:13:02Z

Could add an entry to the docs ? Otherwise this looks good since we require pyserial >= 3.0 and this feature was introduced in 2.5

Added some documentation, hope this is what you were thinking about.

What I am not sure about is how to add a test 🤔

MatthieuDartiailh · 2023-07-21T14:44:09Z

Thanks for the doc update I believe it will do. My only remaining concern is the behavior one gets if pyvisa attempts to read/write an attribute which is not supported for a URL. I did not dive deep enough in pyserial docs to have a clear sense of what would happen then.

As for testing since you can talk to the loop back you could mock a resource no ?

LongnoseRob · 2023-07-22T09:40:43Z

Thanks for the doc update I believe it will do. My only remaining concern is the behavior one gets if pyvisa attempts to read/write an attribute which is not supported for a URL. I did not dive deep enough in pyserial docs to have a clear sense of what would happen then.

If it try an assert_trigger() on a loop:// resource, this causes a NotImplementedError
If I try a read_stb(), this causes a VisaIOError: VI_ERROR_NSUP_OPER (-1073807257): The given session or object reference does not support this operation.

As for testing since you can talk to the loop back you could mock a resource no ?

WIll work on that next

LongnoseRob · 2023-07-22T12:31:34Z

added a unit test, but it seems i cannot trigger the test(s) from the cli on two systems with python 3.11.3 or 3.11.4:

~/builds/pyvisa-py$ python -m unittest pyvisa_py/testsuite/test_serial.py                                                                           |

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK

MatthieuDartiailh · 2023-07-22T17:01:43Z

I may have switched to pytest at one point I will have to double check.

LongnoseRob · 2023-07-23T08:07:23Z

I may have switched to pytest at one point I will have to double check.

pytest seems to work (and also pointed out a deprecatuion issue..

MatthieuDartiailh · 2023-07-24T19:25:13Z

CIs are not good. You may need to skip the new tests in some cases (note I did not check the exact failure).

LongnoseRob · 2023-07-26T10:58:06Z

CIs are not good. You may need to skip the new tests in some cases (note I did not check the exact failure).

The CI failed due to me using .append instead of directly setting the value.
Could you trigger a new CI-run?

LongnoseRob · 2023-07-26T14:01:46Z

Could it be that in the CI enviromnent no pyserial is available and therefore the test fails?

Proof:

Setup a venv wioth the folloing packages:

(ptest) [robby@octoberrain pyvisa_py]$ pip list
Package           Version               Editable project location
----------------- --------------------- ----------------------------
coverage          7.2.7
iniconfig         2.0.0
packaging         23.1
pip               22.3.1
pluggy            1.2.0
pytest            7.4.0
pytest-cov        4.1.0
pytest-cover      3.0.0
pytest-coverage   0.0
PyVISA            1.13.1.dev31+g071fa6d /home/robby/builds/pyvisa
PyVISA-py         0.7.1.dev8+g5e0e5d6   /home/robby/builds/pyvisa-py
setuptools        65.5.0
typing_extensions 4.7.1

run the test:

(ptest) [robby@octoberrain pyvisa_py]$ pytest testsuite/test_serial.py 
============================================================================================================ test session starts ============================================================================================================
platform linux -- Python 3.11.3, pytest-7.4.0, pluggy-1.2.0
rootdir: /home/robby/builds/pyvisa-py
configfile: pyproject.toml
plugins: cov-4.1.0
collected 1 item                                                                                                                                                                                                                            

testsuite/test_serial.py F                                                                                                                                                                                                            [100%]

================================================================================================================= FAILURES ==================================================================================================================
__________________________________________________________________________________________________________ TestSerial.test_serial ___________________________________________________________________________________________________________

self = <pyvisa_py.testsuite.test_serial.TestSerial object at 0x7fbf0e7fbdd0>

    def test_serial(self):
        """Test loop://"""
        msg = b"Test01234567890"
    
        available = ["loop://"]
        expected = []
        exp_missing = []
        missing = {}
    
        rm = ResourceManager("@py")
        try:
            dut = rm.open_resource("ASRLloop://::INSTR")
            print("opened")
            dut.timeout = 3000
            dut.read_termination = "\r\n"
            dut.write_termination = "\r\n"
            dut.write(str(msg))
            ret_val = dut.read()
            if str(msg) == ret_val:
                expected = ["loop://"]
    
        except Exception:
            exp_missing = ["loop://"]
    
>       assert sorted(available) == sorted(expected)
E       AssertionError: assert ['loop://'] == []
E         Left contains one more item: 'loop://'
E         Use -v to get more diff

testsuite/test_serial.py:39: AssertionError

install pyserial

(ptest) [robby@octoberrain pyvisa_py]$ pip list
Package           Version               Editable project location
----------------- --------------------- ----------------------------
coverage          7.2.7
iniconfig         2.0.0
packaging         23.1
pip               22.3.1
pluggy            1.2.0
pyserial          3.5  <----
pytest            7.4.0
pytest-cov        4.1.0
pytest-cover      3.0.0
pytest-coverage   0.0
PyVISA            1.13.1.dev31+g071fa6d /home/robby/builds/pyvisa
PyVISA-py         0.7.1.dev8+g5e0e5d6   /home/robby/builds/pyvisa-py
setuptools        65.5.0
typing_extensions 4.7.1

run test again:

(ptest) [robby@octoberrain pyvisa_py]$ pytest testsuite/test_serial.py 
============================================================================================================ test session starts ============================================================================================================
platform linux -- Python 3.11.3, pytest-7.4.0, pluggy-1.2.0
rootdir: /home/robby/builds/pyvisa-py
configfile: pyproject.toml
plugins: cov-4.1.0
collected 1 item                                                                                                                                                                                                                            

testsuite/test_serial.py .                                                                                                                                                                                                            [100%]

--> We need to extend the CI env to include pyserial for the tests
Can I do this in this PR or should this be done in anotherway?

MatthieuDartiailh · 2023-07-26T17:53:45Z

I am fine with you adding to the CI env but I would also like to see the test behind a conditional skip if pyserial is not available.

LongnoseRob · 2023-07-29T09:32:36Z

I am fine with you adding to the CI env but I would also like to see the test behind a conditional skip if pyserial is not available.

conditional skip included
enviroment setup for tests includes now pyserial

MatthieuDartiailh

Looks good. I just have a nitpick regarding the CI configuration.

MatthieuDartiailh · 2023-07-29T13:17:42Z

azure-pipelines.yml

@@ -32,9 +32,12 @@ steps:
      source $HOME/miniconda3/bin/activate
      echo Activate environment
      call conda activate test_
+      echo Install support packages
+      pip install pyserial
      echo Install project
      pip install git+https://github.com/pyvisa/pyvisa.git#egg=pyvisa
      pip install -e .


You could do pip install -e .[serial] instead with a comment explining why we install this extra.

If you would prefer it that way, I will change it.

codecov-commenter · 2023-07-29T13:19:17Z

Codecov Report

Merging #386 (cdd8d32) into main (c33f4f2) will decrease coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 18.51%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   23.05%   23.02%   -0.03%     
==========================================
  Files          22       23       +1     
  Lines        3292     3318      +26     
  Branches      456      457       +1     
==========================================
+ Hits          759      764       +5     
- Misses       2516     2536      +20     
- Partials       17       18       +1

Flag	Coverage Δ
unittests	`23.02% <18.51%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
pyvisa_py/serial.py	`5.71% <0.00%> (+0.02%)`	⬆️
pyvisa_py/testsuite/test_serial.py	`19.23% <19.23%> (ø)`

... and 1 file with indirect coverage changes

azure-pipelines.yml

LongnoseRob · 2023-07-29T14:37:47Z

Thanks for the review.

replace the call to Serial.serial with serial.serial_for_url, in order

d17258c

to support loop:// and other url-types supported in pyserial (>3.0)

documentation update

10f052e

initial test-addition

6c3e83d

LongnoseRob mentioned this pull request Jul 23, 2023

protocols/rpc.py:26: DeprecationWarning: 'xdrlib' is deprecated #387

Open

fix the testsuite

5e0e5d6

LongnoseRob added 2 commits July 29, 2023 18:24

add conditional skip if PySerial is not found

570bf02

add pyserial as package avialable for the test environment

e175de1

MatthieuDartiailh reviewed Jul 29, 2023

View reviewed changes

LongnoseRob added 2 commits July 29, 2023 22:42

change extra package installation

f2f92c0

Update CHNAGES file

43f04c9

MatthieuDartiailh requested changes Jul 29, 2023

View reviewed changes

azure-pipelines.yml Outdated Show resolved Hide resolved

azure-pipelines.yml Outdated Show resolved Hide resolved

implement code-review changes

cdd8d32

MatthieuDartiailh merged commit 8016ec0 into pyvisa:main Jul 29, 2023
15 checks passed

LongnoseRob deleted the loop branch July 30, 2023 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace the call to serial.Serial with serial.serial_for_url #386

replace the call to serial.Serial with serial.serial_for_url #386

LongnoseRob commented Jul 20, 2023 •

edited

Loading

MatthieuDartiailh commented Jul 20, 2023

LongnoseRob commented Jul 21, 2023 •

edited

Loading

MatthieuDartiailh commented Jul 21, 2023

LongnoseRob commented Jul 22, 2023

LongnoseRob commented Jul 22, 2023 •

edited

Loading

MatthieuDartiailh commented Jul 22, 2023

LongnoseRob commented Jul 23, 2023

MatthieuDartiailh commented Jul 24, 2023

LongnoseRob commented Jul 26, 2023 •

edited

Loading

LongnoseRob commented Jul 26, 2023

MatthieuDartiailh commented Jul 26, 2023

LongnoseRob commented Jul 29, 2023

MatthieuDartiailh left a comment

MatthieuDartiailh Jul 29, 2023

LongnoseRob Jul 29, 2023

codecov-commenter commented Jul 29, 2023 •

edited

Loading

LongnoseRob commented Jul 29, 2023

replace the call to serial.Serial with serial.serial_for_url #386

replace the call to serial.Serial with serial.serial_for_url #386

Conversation

LongnoseRob commented Jul 20, 2023 • edited Loading

MatthieuDartiailh commented Jul 20, 2023

LongnoseRob commented Jul 21, 2023 • edited Loading

MatthieuDartiailh commented Jul 21, 2023

LongnoseRob commented Jul 22, 2023

LongnoseRob commented Jul 22, 2023 • edited Loading

MatthieuDartiailh commented Jul 22, 2023

LongnoseRob commented Jul 23, 2023

MatthieuDartiailh commented Jul 24, 2023

LongnoseRob commented Jul 26, 2023 • edited Loading

LongnoseRob commented Jul 26, 2023

MatthieuDartiailh commented Jul 26, 2023

LongnoseRob commented Jul 29, 2023

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

MatthieuDartiailh Jul 29, 2023

Choose a reason for hiding this comment

LongnoseRob Jul 29, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jul 29, 2023 • edited Loading

Codecov Report

LongnoseRob commented Jul 29, 2023

LongnoseRob commented Jul 20, 2023 •

edited

Loading

LongnoseRob commented Jul 21, 2023 •

edited

Loading

LongnoseRob commented Jul 22, 2023 •

edited

Loading

LongnoseRob commented Jul 26, 2023 •

edited

Loading

codecov-commenter commented Jul 29, 2023 •

edited

Loading