-
Notifications
You must be signed in to change notification settings - Fork 54
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 RobotFramework test for c8y-firmware-plugin #1787
Add a RobotFramework test for c8y-firmware-plugin #1787
Conversation
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Robot Results
Passed Tests
|
Execute Command mkdir -p /etc/tedge/operations/c8y/${CHILD_SN} | ||
ThinEdgeIO.Transfer To Device ${CURDIR}/c8y_Firmware /etc/tedge/operations/c8y/${CHILD_SN}/ | ||
ThinEdgeIO.Restart Service tedge-mapper-c8y | ||
Cumulocity.Device Should Exist ${CHILD_SN} |
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 belive this sequence is causing the child device to be registered implicity resulting in the child device name having the MQTT Device
prefix in the name.
For example the first test run had a failure when validating the child device name:
TST_rev_dense_manuscript != MQTT Device TST_rev_dense_manuscript
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.
Ah horrible, race condition. That means some SmartREST message was published to the child device before tedge-mapper-c8y
sends 101 to create a child device with appropriate inventory fragments :/
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.
Reordering it "may" fix it, however it shows that we still have an initialization problem, though it does not look to be related to this feature specifically.
Execute Command mkdir -p /etc/tedge/operations/c8y/${CHILD_SN} | |
ThinEdgeIO.Transfer To Device ${CURDIR}/c8y_Firmware /etc/tedge/operations/c8y/${CHILD_SN}/ | |
ThinEdgeIO.Restart Service tedge-mapper-c8y | |
Cumulocity.Device Should Exist ${CHILD_SN} | |
Execute Command mkdir -p /etc/tedge/operations/c8y/${CHILD_SN} | |
ThinEdgeIO.Restart Service tedge-mapper-c8y | |
Cumulocity.Device Should Exist ${CHILD_SN} | |
ThinEdgeIO.Transfer To Device ${CURDIR}/c8y_Firmware /etc/tedge/operations/c8y/${CHILD_SN}/ |
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.
Yep this old chestnut.
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.
Yeah, first 114
(declaring supported operation) is arrived, then 101
(creating a child device) is arrived in c8y. Should be opposite order.
After a couple of trials, I figure out that we don't need a restart. Instead, sleep 3s
after Set Device Context
helps. From this, I started feeling that the issue is in testing setup environment side. Therefore, I decided not to create an issue yet.
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.
Yeah, first
114
(declaring supported operation) is arrived, then101
(creating a child device) is arrived in c8y. Should be opposite order.After a couple of trials, I figure out that we don't need a restart. Instead,
sleep 3s
afterSet Device Context
helps. From this, I started feeling that the issue is in testing setup environment side. Therefore, I decided not to create an issue yet.
Though today a "sleep 3s" helps, tomorrow it doesn't...makes for a flakey test. I would much prefer to create a dynamic wait instead.
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.
Though today a "sleep 3s" helps, tomorrow it doesn't...makes for a flakey test. I would much prefer to create a dynamic wait instead.
Definitely dynamic wait makes much more sense! Sleep with static seconds is just a workaround...
The test passed with the magic |
Library ../../../.venv/lib/python3.9/site-packages/robot/libraries/String.py | ||
Library ../../../.venv/lib/python3.9/site-packages/robot/libraries/OperatingSystem.py | ||
Library ../../../.venv/lib/python3.9/site-packages/robot/libraries/DateTime.py |
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.
Are you actually using keywords from these Libraries?
If so we should be using the normal imports like, Library String
and not the relative path. Though I don't we need most of those as we should have another keyword inside ThinEdgeIO library.
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.
Good catch. It needs String
for Get Regexp Matches
only. Fixed.
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
8f5d1fe
to
827309b
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.
Approved
* Add a RobotFramework test for c8y-firmware-plugin Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Proposed changes
Create a new test file for c8y-firmware-plugin. Prepared two scenarios:
Note:
c8y-firmware-plugin
must be installed aftertedge-agent
, otherwise the installation ofc8y-firmware-plugin
will fail due to the failure to create directories under/var/tedge
byc8y-firmware-plugin --init
. That's why the change on the bootstrap script looks a bit strange.Types of changes
Paste Link to the issue
#1696
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments