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.

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

unittests: Move at test to driver tests #20540

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

Teufelchen1
Copy link
Contributor

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
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

Copy link
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

riot-ci commented Apr 5, 2024

Murdock results

✔️ PASSED

ac40354 tests/drivers/at_unit: Add Makefile.ci

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

Artifacts

@Teufelchen1
Copy link
Contributor Author

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
Copy link
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
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
Member

chrysn commented Apr 5, 2024

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

@miri64
Copy link
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
25 checks passed
@derMihai
Copy link
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
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
Development

Successfully merging this pull request may close these issues.

7 participants