Skip to content

unittests: Move at test to driver tests#20540

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
Teufelchen1:fix/at_unittest
Apr 5, 2024
Merged

unittests: Move at test to driver tests#20540
miri64 merged 2 commits intoRIOT-OS:masterfrom
Teufelchen1:fix/at_unittest

Conversation

@Teufelchen1
Copy link
Copy Markdown
Contributor

@Teufelchen1 Teufelchen1 commented Apr 4, 2024

Contribution description

Hi 🦜

This moves the unit test for the at driver into the folder for driver tests. The test accesses/depends on hardware which is not valid for a unit test.

Further, the original test disabled UART_DEV(0) which I changed to UART_DEV(1). This is needed as some boards use device zero for stdio and you can not have a good testing experience without stdio.
Please provide feedback on this workaround, I am unsure on the quality of it.

Testing procedure

Running make -C tests/drivers/at_unit/ all flash should provide this output upon success:

# main(): This is RIOT! 
# AT unit-like test
# 
# .......
# OK (7 tests)

Issues/PRs references

Introduced in commit 63e057c by PR #20423

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Apr 4, 2024
@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Apr 4, 2024

you are right even thought the test itself does not make use of the uart the driver/at that it tests depend on it

@derMihai might have some thoughts

chrysn
chrysn approved these changes Apr 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

It's weird that there is a device involved that just gets turned off, but aparently that's how at_dev_init works (can't pass it the none-device), but that's beyond the scope of fixing it.

I don't know the precise workings of the AT module or this test, but as the test passes (doing things) for you, I think it's good.

@chrysn chrysn 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 labels Apr 5, 2024
@Teufelchen1 Teufelchen1 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 Apr 5, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Apr 5, 2024 •
edited
Loading

Murdock results

✔️ PASSED

ac40354 tests/drivers/at_unit: Add Makefile.ci

Success Failures Total Runtime
35 0 35 01m:02s

Artifacts

@Teufelchen1
Copy link
Copy Markdown
Contributor Author

Teufelchen1 commented Apr 5, 2024

C is such a magic. A test that compiles and runs fine on my machine, suddenly misses a header on the CI.
@chrysn, I won't be able to fix that today (or even this weekend) could you fix up my branch? 👉 👈 Hardfreeze is on monday...

@chrysn chrysn force-pushed the fix/at_unittest branch from 459b25d to 0ddbba3 Compare April 5, 2024 11:17
@chrysn
Copy link
Copy Markdown
Member

chrysn commented Apr 5, 2024

Fixup passed. Let's try if I can force push too (not sure it really worked); at any rate, I'll let you do the merge pushing so you confirm my one-line addition.

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Apr 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 5, 2024
@miri64 miri64 enabled auto-merge April 5, 2024 12:05
@miri64 miri64 added this pull request to the merge queue Apr 5, 2024
@miri64 miri64 removed this pull request from the merge queue due to a manual request Apr 5, 2024
@miri64
Copy link
Copy Markdown
Member

miri64 commented Apr 5, 2024

Ah, please update the Makefile.ci with the arduino-duemilanove. It is too large for that board.

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Apr 5, 2024

This'll need Makefile.ci, building it already.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Apr 5, 2024

(maybe even more, but the full build stopped there)

@miri64 miri64 enabled auto-merge April 5, 2024 12:08
@miri64 miri64 added this pull request to the merge queue Apr 5, 2024
Merged via the queue into RIOT-OS:master with commit f167618 Apr 5, 2024
@derMihai derMihai mentioned this pull request Apr 8, 2024
Merged
@derMihai
Copy link
Copy Markdown
Contributor

derMihai commented Apr 8, 2024

This PR does break a few things, see #20551.

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrysn chrysn chrysn approved these changes

@miri64 miri64 miri64 approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

Release 2024.04

Development

Successfully merging this pull request may close these issues.

7 participants

@Teufelchen1 @kfessel @riot-ci @chrysn @miri64 @derMihai @MrKevinWeiss