Skip to content
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.

Sign up for GitHub

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

Jump to bottom

netdev: Provide test network device with netdev interface #1731

Merged
merged 2 commits into from
Oct 30, 2014
Merged

netdev: Provide test network device with netdev interface #1731

merged 2 commits into from
Oct 30, 2014

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Sep 29, 2014

The reasoning behind this PR is twofold:

  1. test the "lower" part (the device part) of netdev
  2. provide a well defined testing infrastructure for modules that utilises netdev

To test this, run

make -C tests/unittests tests-netdev_dummy flash term

Depends on #1492: (it's merged)

* #1680
|\
… …
| |
| * #1448
| |\
* | | #1733
| * | #1732
| | * #1731
| |/
|/
* (master)

@LudwigKnuepfer LudwigKnuepfer added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 29, 2014
@N8Fear
Copy link

N8Fear commented Sep 29, 2014

Is there anything sensible to be done about:

drivers/include/netdev/base.h:327: style (unusedStructMember): struct or union member 'netdev_t::type' is never used.

If not and the member is needed I think the warning should be suppressed.

@miri64
Copy link
Member Author

miri64 commented Sep 29, 2014

Yes, it is used in #1733 for example. Since this PR is thought to simplify the review of #1492 I think this is not necessary.

@miri64 miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: network Area: Networking Area: tests Area: tests and testing framework and removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Sep 29, 2014
This was referenced Sep 29, 2014
Merged
Closed
Closed
@miri64 miri64 force-pushed the netdev-test-dummy branch from 30d53a0 to a804889 Compare October 1, 2014 17:25
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 2, 2014
@miri64 miri64 force-pushed the netdev-test-dummy branch from a804889 to 559964d Compare October 2, 2014 10:33
@miri64
Copy link
Member Author

miri64 commented Oct 2, 2014

Rebased to master

@miri64 miri64 added State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Oct 2, 2014
@miri64 miri64 added this to the Release NEXT MAJOR milestone Oct 3, 2014
@miri64 miri64 force-pushed the netdev-test-dummy branch 2 times, most recently from 876e7b9 to 559964d Compare October 3, 2014 13:22
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 3, 2014
@LudwigKnuepfer
Copy link
Member

I think there should be no random element in a unittest.

@LudwigKnuepfer
Copy link
Member

This test does not change the API, right? (remove label if agree)

@miri64 miri64 removed the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Oct 16, 2014
@miri64
Copy link
Member Author

miri64 commented Oct 16, 2014

Done.

@miri64
Copy link
Member Author

miri64 commented Oct 16, 2014

@LudwigOrtmann You won't believe how much errors I already found because of random elements in unittests ;-)

@LudwigKnuepfer
Copy link
Member

I want a telephone joker.

@miri64
Copy link
Member Author

miri64 commented Oct 16, 2014

☎️ 🃏 😜

@LudwigKnuepfer
Copy link
Member

Thank ye!

@LudwigKnuepfer
Copy link
Member

But seriously, I think unit tests must be reproducible.
So, even if (and I'm not in favor of that) they include a per-run random value, it at least has to be printed out as part of the test result.

@LudwigKnuepfer
Copy link
Member

(I would happily ACK some fuzzer test external to the unit tests for selected interfaces.)

@miri64
Copy link
Member Author

miri64 commented Oct 16, 2014

Happy?

@LudwigKnuepfer
Copy link
Member

Medium .. I would have retained the macros (maybe renaming them) in tests/unittests/tests-netdev_dummy/tests-netdev_dummy.c, but defined them statically (maybe even in the Makefile). Since some of the tests use the same value twice, macros would be a good thing here.

Either way - you should remove the shell commands from tests/unittests/tests-netdev_dummy/Makefile.

@miri64
Copy link
Member Author

miri64 commented Oct 17, 2014

Done

@miri64
Copy link
Member Author

miri64 commented Oct 19, 2014

I don't get why Travis isn't building for the telosb, wsn430-v1_3b, wsn430-v1_4 and z1 :(

@OlegHahm
Copy link
Member

rebase required

@miri64 miri64 force-pushed the netdev-test-dummy branch from c3813f4 to d98c125 Compare October 29, 2014 13:58
@miri64
Copy link
Member Author

miri64 commented Oct 29, 2014

Done

LudwigKnuepfer
LudwigKnuepfer reviewed Oct 29, 2014
View reviewed changes
tests/unittests/Makefile
@@ -29,11 +44,23 @@ DISABLE_MODULE += auto_init
# Pull in `Makefile.include`s from the test suites:
-include $(UNIT_TESTS:%=$(RIOTBASE)/tests/unittests/%/Makefile.include)

ifneq (,$(filter netdev_dummy,$(USEMODULE)))
USEMODULE += netdev_base
DIRS += netdev_dummy
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

netdev_dummy is a module that is only really useful or sensible in the context of unittests. So why not define it's dependencies in the Makefile of unittests?

Copy link
Member

Choose a reason for hiding this comment

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

Because it has no advantage and deviates from standard practice.

Copy link
Member

Choose a reason for hiding this comment

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

But: I overlooked, that the interface is actually part of the tests/unittests structure, so I'm fine with it staying as it is.

@LudwigKnuepfer
Copy link
Member

OK, squash and off we go =)

@LudwigKnuepfer LudwigKnuepfer added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 30, 2014
@LudwigKnuepfer
Copy link
Member

Uck

@miri64 miri64 force-pushed the netdev-test-dummy branch from 32847ee to 0de7883 Compare October 30, 2014 19:36
miri64 added 2 commits October 30, 2014 20:37
@miri64
unittests: add test network device
cf3fdaa 
The reasoning behind this commit is twofold:
1. test the "lower" part (the device part) of netdev
2. provide a well defined testing infrastructure for modules that utilise
   netdev
The actual tests that represent 1. are provided in the following commit
@miri64
unittests: Provide unittests for net_dev_dummy
0de7883 
* tests if net_dev_dummy is correct and if driver part of net_dev is
  correct
miri64 added a commit that referenced this pull request Oct 30, 2014
@miri64
Merge pull request #1731 from authmillenon/netdev-test-dummy
3bfbc60 
netdev: Provide test network device with netdev interface
@miri64 miri64 merged commit 3bfbc60 into RIOT-OS:master Oct 30, 2014
@miri64 miri64 deleted the netdev-test-dummy branch October 30, 2014 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@LudwigKnuepfer LudwigKnuepfer

Labels
Area: network Area: Networking Area: tests Area: tests and testing framework CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable
Projects
None yet
Milestone
Release 2014.12
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @N8Fear @LudwigKnuepfer @OlegHahm