Skip to content

tests/libcose: Move from unittests to regular test#10183

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/tests/libcose_move
Oct 18, 2018
Merged

tests/libcose: Move from unittests to regular test#10183
miri64 merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/tests/libcose_move

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Oct 17, 2018 •
edited by cladmi
Loading

Contribution description

Moves the libcose unittest to a "regular" test in pkg_libcose. saves quite a bit of flash and ram on a samr21-xpro for the unittests (No, still doesn't fit):
Before:

region `rom' overflowed by 159324 bytes
region `ram' overflowed by 17180 bytes

After:

region `rom' overflowed by 131664 bytes
region `ram' overflowed by 12100 bytes

Testing procedure

Run the test in pkg_libcose.

Issues/PRs references

Part of #10187

@bergzand bergzand added Area: tests Area: tests and testing framework Area: pkg Area: External package ports Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 17, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 17, 2018

😆 to

(No, still doesn't fit)

Not the PR itself ;-)

@miri64 miri64 self-assigned this Oct 17, 2018
@miri64 miri64 added 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 and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 17, 2018
miri64
miri64 reviewed Oct 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

LGTM. One question though.

miri64
miri64 approved these changes Oct 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Murdock still requires some BOARD_INSUFFIENT_MEMORY to be set. You can squash that immediately

@bergzand bergzand force-pushed the pr/tests/libcose_move branch from c0c2c5b to 31da48d Compare October 17, 2018 19:41
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 17, 2018

Should be fixed now

miri64
miri64 requested changes Oct 17, 2018
View reviewed changes
tests/pkg_libcose/tests/01-run.py Outdated


def testfunc(child):
child.expect_exact('OK (3 tests)')
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe make this a regex. This way the test is extensible (and might not fail randomly ;-))

Suggested change
child.expect_exact('OK (3 tests)')
child.expect(r'OK (\d+ tests)')

Copy link
Copy Markdown
Member Author

@bergzand bergzand Oct 17, 2018

Choose a reason for hiding this comment

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

Agreed, Fixed

This was referenced Oct 17, 2018
Merged
Merged
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 17, 2018

I think I have to increase the timeout on the CI run test.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 17, 2018

That could be the problem. Let me test this on a more accessible (i.e. iotlab) samr21

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 17, 2018

Seems saclay is currently down :-/.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 17, 2018

(their was some maintainance on IoT-LAB announced for today)

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 17, 2018

Seems saclay is currently down :-/.

Ah, that means it's not only down for me.

Testing it locally shows that the test takes around 67 seconds:

2018-10-17 22:28:20,717 - INFO # main(): This is RIOT! (Version: 2018.10-devel-1213-g386c9-dystopia-pr/tests/libcose_move)
2018-10-17 22:29:27,138 - INFO # ...
2018-10-17 22:29:27,140 - INFO # OK (3 tests)

Do we actually want to have this run on the CI if it takes this long?

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 17, 2018

Increased the timeout to 120s

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 17, 2018

Do we actually want to have this run on the CI if it takes this long?

Good question. IMHO for nightly builds this should be done, but maybe not for every PR that sets CI: run tests. I don't know (except with re-introducing not running/compiling uneffected test) this is possible right now.

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 17, 2018

@kaspar030 @smlng Do you guys have an opinion on this?

@bergzand bergzand mentioned this pull request Oct 17, 2018
Open
16 tasks
smlng
smlng approved these changes Oct 18, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

tested ACK, please squash!

This was referenced Oct 18, 2018
Merged
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 18, 2018

An answer to our question would be nice though ;-)

@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 18, 2018

I like this, see #10188 and #10189 😉 IIRC I had a similar discussion with others (e.g. @cladmi) to finally have packages tests within the package directory and make them self-contained. So moving the tests out of unittests is a first step in that direction and also has the benefit of reducing unittests binary size.

TL;DR: 👍 🎉 ❤️

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 18, 2018

But the question was if we want to introduce a 2min wait-time to the CI tests, because this test requires two minutes to finish on samr21-xpro.

@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 18, 2018

But the question was if we want to introduce a 2min wait-time to the CI tests, because this test requires two minutes to finish on samr21-xpro.

ah that question: well, I would say no don't run the tests on hardware if it takes that long

miri64
miri64 reviewed Oct 18, 2018
View reviewed changes
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2018

I've extended the test script a bit to only use the 120 seconds timeout when running on non-native. Now when the test breaks, the CI doesn't have to wait for the 2 minute timeout to pass on native.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Oct 18, 2018

@smlng we discussed this IRL a few month ago with @bergzand and he proposed to move them out of unittest as it will simplify moving them to pkg later. And it is indeed a first step in that direction.

miri64
miri64 approved these changes Oct 18, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK, please squash

@bergzand bergzand force-pushed the pr/tests/libcose_move branch from ea3312e to 03e2eca Compare October 18, 2018 09:43
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2018

squashed!

@bergzand bergzand mentioned this pull request Oct 18, 2018
Closed
@bergzand bergzand added 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 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 Oct 18, 2018
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2018

Ready to merge here?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 18, 2018

Yepp

@miri64 miri64 merged commit ed1eb8e into RIOT-OS:master Oct 18, 2018
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Oct 18, 2018

Thanks for reviewing!

@bergzand bergzand deleted the pr/tests/libcose_move branch October 18, 2018 13:18
@smlng smlng mentioned this pull request Oct 19, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

+1 more reviewer

@smlng smlng smlng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@miri64 miri64

Labels

Area: pkg Area: External package ports 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 CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bergzand @miri64 @smlng @cladmi