Skip to content

tests: provide tests for emcute#11823

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
miri64:tests/enh/emcute
Feb 20, 2020
Merged

tests: provide tests for emcute#11823
miri64 merged 1 commit intoRIOT-OS:masterfrom
miri64:tests/enh/emcute

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Jul 9, 2019

Contribution description

This provides a test application for emcute based on the MQTT-SN layer for scapy I provided in #11822. Since it uses UDP sockets it can be run without root, but requires scapy nonetheless.

Attached to this PR are fixes that I found with these tests.

Fixes

  • emcute: fix payload copy error for emcude_pub
    len is used with the memcpy() to copy the payload to tbuf. With a
    payload provided that is just long enough to fill tbuf, len += 6
    leads to the memcpy() overriding data after tbuf (e.g. the
    mutex that is unlocked right after) and thus resulting in potential
    segmentation faults.
    Additionally + 6 can only be applied if the total packet length is
    below 256 (see spec), so len + pos is what needs to be provided to the
    corresponding send functions instead (pos adapts to the header length
    of the PUBLISH message)
    (now in emcute: fix payload copy error for emcute_pub #11957)
  • emcute: fix length field calculation
    The length field in an MQTT packet carries the total length of the
    packet. If it is below 256 (i.e. fits in one byte) only one byte is
    used for the length field. If it is larger than that 3 bytes are used,
    with the first byte having the value 0x01 and the remaining bytes
    representing the length in as a 2 byte unsigned integer in network byte
    order. Resulting from that it can be assessed that the check in
    emcutes's set_len() function is wrong as it needs to be checked if
    len is lesser or equal to 0xff - 1. len <= (0xff - 1) can be
    simplified to len < 0xff. For some larger packages this safes 2 bytes
    of wasted packet space.
    (now in emcute: fix length field calculation #11958)

Testing procedure

Make sure you have a recent version of scapy for python3 installed.

Run

BOARD="<your choice>" make -C all test

(the blacklisted boards are excluded of course).

Issues/PRs references

Requires on #11822.

@miri64 miri64 added Area: network Area: Networking Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 9, 2019
@miri64 miri64 added this to the Release 2019.10 milestone Jul 9, 2019
@miri64 miri64 requested a review from haukepetersen July 9, 2019 14:42
@miri64 miri64 changed the title Tests/enh/emcute tests: provide tests for emcute Jul 9, 2019
@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 9, 2019
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 10, 2019

Ok, some details on the test server now that I cleaned it up a bit: it is a scapy automaton (basically a deterministic automaton with sockets and packet parsing ability) and is defined as follows.

mqttsn_test_server

You can generate this graphic yourself, using the following within a reachable portion of the test script:

    MQTTSNServer.graph(options="-Gratio=.4", format="png",
            target="mqttsn_test_server.png")

This allows me to test some expected packet exchanges and include also some unexpected data exchanges. If the automaton reaches an error state, an exception is thrown. If it reaches the END state, the automaton's run() method stops.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 5, 2019

I split out the two bug fixes into their own PRs.: #11957 and #11958.

@miri64 miri64 force-pushed the tests/enh/emcute branch 2 times, most recently from c321de1 to b8c22cc Compare August 9, 2019 13:15
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 9, 2019

I rebased and squashed to current master, since I closed #11822 due to scapy 2.4.3 being out (which includes that submodule). I also adopted the tests to include the submodule properly. Lastly, I removed the two bug fixes, as I moved them out into their own PRs, as I mentioned earlier.

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 9, 2019
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 7, 2019
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 7, 2019

Adapted for #12382.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 7, 2019

Rebased to current master to incorporate latest fixes.

@miri64 miri64 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Oct 7, 2019
@kb2ma kb2ma modified the milestones: Release 2019.10, Release 2020.01 Oct 9, 2019
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Since this hasn't gotten any reviews yet, just squash.
There is nothing controversial about additional tests, this this should be able to go in fast.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 19, 2020

Rebased and squashed to current master, left Makefile.ci out for now, to get a more current list of files.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 19, 2020

Rebased and squashed to current master, left Makefile.ci out for now, to get a more current list of files.

Updated and also fixed some static errors and required adaptations for current master.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 19, 2020

Not sure what happened, but I missed some boards somehow...

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 19, 2020
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 19, 2020

Test failure unrelated.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Feb 19, 2020
@benpicco
Copy link
Copy Markdown
Contributor

I wonder what's wrong with the micropython test. It keeps failing for samr21-xpro quite consistently.
When I run it locally, it succeeds.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 19, 2020

Maybe its not the test, but the test + a certain worker within Murdock that fails. The set of test runner workers is (due to the hardware requirements) quite small.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Feb 20, 2020

Murdock is happy now.

Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Can't say much about the content of the tests, but they already caught some bugs so we should really have them in to prevent new bugs from sneaking in.

@miri64 miri64 merged commit ea3296d into RIOT-OS:master Feb 20, 2020
@miri64 miri64 deleted the tests/enh/emcute branch February 20, 2020 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants