Added fixes to snap7/client.py read_area function to allow for Counter and Timer Reads #121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…r and Timer reads.
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
==========================================
+ Coverage 78.67% 78.91% +0.24%
==========================================
Files 15 15
Lines 2068 2092 +24
==========================================
+ Hits 1627 1651 +24
Misses 441 441
Continue to review full report at Codecov.
|
|
hi @JKelly-Mevex ! Thanks for your PR! Do you think it is possible to modify the test suite so it covers the conditions using the simulator? |
|
I can take a stab out it if you can give me some pointers on which area of
the test suite I would need to focus on.
Not sure how the simulator works. Is there something I can read to bring me
up to speed on this.
Please advise
James
…On Mon, May 27, 2019 at 7:49 AM Gijs Molenaar ***@***.***> wrote:
hi @JKelly-Mevex <https://github.com/JKelly-Mevex> ! Thanks for your PR!
Do you think it is possible to modify the test suite so it covers the
conditions using the simulator?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#121?email_source=notifications&email_token=AAKQIYKBOLMIGCMHPSQY4Y3PXPDDJA5CNFSM4HP22UO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJTBFY#issuecomment-496185495>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQIYJVSUB3LTCE7T7LDMTPXPDDJANCNFSM4HP22UOQ>
.
|
|
Currently, the https://github.com/gijzelaerr/python-snap7/blob/master/test/test_client.py#L534 What would be amazingly perfect if there is an extra test function just like the other unit tests in that file that would verify that each condition in the condition you added work as intented. You can run the test suite with for example nosetests. |
|
I will take a peek at this tomorrow and see what I can do. I make have a
question or two once I get looking at
it.
Thanks
James
…On Mon, May 27, 2019 at 9:14 AM Gijs Molenaar ***@***.***> wrote:
Currently, the fullupload client function is completely untested:
https://github.com/gijzelaerr/python-snap7/blob/master/test/test_client.py#L534
What would be amazingly perfect if there is an extra test function just
like the other unit tests in that file that would verify that each
condition in the condition you added work as intented.
You can run the test suite with for example nosetests.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#121?email_source=notifications&email_token=AAKQIYPQPZDP6KXF3DMJQVDPXPNCFA5CNFSM4HP22UO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJYY5Y#issuecomment-496209015>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQIYNXP4BEYQFGE3R2GU3PXPNCFANCNFSM4HP22UOQ>
.
|
|
Hi,
I noticed the following when looking at the test_client.py:
ip = '127.0.0.1'
tcpport = 1102
db_number = 1
rack = 1
slot = 1
What simulator are you using?
Please advise
James
…On Mon, May 27, 2019 at 9:14 AM Gijs Molenaar ***@***.***> wrote:
Currently, the fullupload client function is completely untested:
https://github.com/gijzelaerr/python-snap7/blob/master/test/test_client.py#L534
What would be amazingly perfect if there is an extra test function just
like the other unit tests in that file that would verify that each
condition in the condition you added work as intented.
You can run the test suite with for example nosetests.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#121?email_source=notifications&email_token=AAKQIYPQPZDP6KXF3DMJQVDPXPNCFA5CNFSM4HP22UO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJYY5Y#issuecomment-496209015>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQIYNXP4BEYQFGE3R2GU3PXPNCFANCNFSM4HP22UOQ>
.
--
James M Kelly
Mevex Corporation
108 Willowlea Rd, Box 1778,
Stittsville, ON, K2S-1B4, Canada
TEL: 613-831-2664
MOBILE: 613-697-4357
FAX: 613-831-3648
Email: jkelly@mevex.com
P please consider the environment before printing this email
|
|
During test initialisation https://github.com/gijzelaerr/python-snap7/blob/master/test/test_client.py#L29
https://github.com/gijzelaerr/python-snap7/blob/master/snap7/bin/snap7-server.py |
|
Without making any changes to the test code, this is the results that I am
getting when I run the test_client.py:
[image: Screenshot from 2019-05-28 04-39-26.png]
…On Tue, May 28, 2019 at 3:52 AM Gijs Molenaar ***@***.***> wrote:
During test initialisation
https://github.com/gijzelaerr/python-snap7/blob/master/test/test_client.py#L29
bin/snap7-server.py is started in a different process. This script is
nothing more than a server emulator, which is part of the snap7 library.
https://github.com/gijzelaerr/python-snap7/blob/master/snap7/bin/snap7-server.py
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#121?email_source=notifications&email_token=AAKQIYOWP5WD32WY4KQXUSTPXTQELA5CNFSM4HP22UO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWLI2HA#issuecomment-496405788>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQIYMD6H6YJRHNDOVMDMTPXTQELANCNFSM4HP22UOQ>
.
--
James M Kelly
Mevex Corporation
108 Willowlea Rd, Box 1778,
Stittsville, ON, K2S-1B4, Canada
TEL: 613-831-2664
MOBILE: 613-697-4357
FAX: 613-831-3648
Email: jkelly@mevex.com
P please consider the environment before printing this email
|
|
Sorry but i don't see any image in the github web interface. |
|
[image: Screenshot from 2019-05-28 04-39-26.png]
…On Tue, May 28, 2019 at 4:46 AM Gijs Molenaar ***@***.***> wrote:
Sorry but i don't see any image in the github web interface.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#121?email_source=notifications&email_token=AAKQIYPSW73LA4G7XGVPO4TPXTWOXA5CNFSM4HP22UO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWLNIRA#issuecomment-496424004>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKQIYPJMOIAEBY2UT4HICTPXTWOXANCNFSM4HP22UOQ>
.
|
|
Sorry, the image is not working here so I am cutting the terminal output below: |
|
how do you run the test suite on what platform? Do you use a virtualenv? this works for me on OSX: I'm confused by the missing |
|
I am running this on Linux. I'll double check that this is pointing towards the latest and greatest untouched code. I might roll a virtual environment as well to save some hassle. I'll get back to you on this. |
|
I got the virtual environment setup as per your instructions above. The first time I executed ./run_tests.sh, I |
|
Yes, that looks fine. The verbose logging comes from the server. |
|
Ok. I have not authored a unittest before so will read up on it as it's something I should learn anyway. |
|
It is not hard, just have a look at the other unit tests. Just calling the function without errors is already a great start. Thanks for looking into this. |
|
Looking at the already existing test for read_area, it would seem that I just need to add two extra cases: 1 for a counter and one for a timer. Seem correct to you? (Existing unmodified test below) def test_read_area(self): |
|
Note that we are testing the Sorry i don't have time left to look at this now. |
|
One final question. I made changes to the read_area function however you want me to write a test for the full_upload function? Please confirm. |
|
You have modifed the thanks |
|
Sorry, I see what is going on now. You had me confused for a bit there. So, looking in Github at the file differences, the display in misleading. If you look at the line numbers in the affected code, it jumps from 179 to 250. At first glance, it gives the impression that the code change occurred in full_upload. If you click on the blue expand buttons and expand it out, you will see that my code changes were actually in the read_area function. |
|
Well spotted! You are rights, sorry about that. I hope you know enough now :) |
|
I'm going to still keep on learning about the unit tests so perhaps I can contribute in the future more. If you still want a test authored for full_upload, I am willing to take a stab at it. Did you want me to test the extra conditions in read_area now to test for both counter and timer reads? |
|
any contributions are welcome! I'm just the maintainer of this project but have not used it for a while. I'm just coordinating the PR's for the benefit of the world :) |
…ounters and timers. Updated test_client to test both reading and writing to timers and counters in read_area and write_area.
|
I made changes to both read_area and write_area now. I have tested my changes on a real plc and they are working now (timers and counters). I also updated the server file to create timer and counter areas and have modified the tests for read_area and write_area to test for DB's, CT's, and TM's. I might take a stab at the full upload test at a later date. |
gijzelaerr
approved these changes
May 31, 2019
|
looking good! thanks!! |
|
No problem. I'll try to test some more when I can and get coverage on some of the missing tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.