Skip to content

tests/feature_resolution: add build system unit tests#15973

Merged
fjmolinas merged 3 commits intoRIOT-OS:masterfrom
maribu:feature-resolution-test
Feb 13, 2021
Merged

tests/feature_resolution: add build system unit tests#15973
fjmolinas merged 3 commits intoRIOT-OS:masterfrom
maribu:feature-resolution-test

Conversation

@maribu
Copy link
Member

@maribu maribu commented Feb 9, 2021 •
edited
Loading

Contribution description

Detecting bugs in feature resolution previously resulted in a lot of manual testing and more than once an issue slipped through. Dedicated unit tests should fix both.

Testing procedure

Hopefully, it is indeed as trivial as I had hoped for and Murdock will just run the test. The issue described in #15967 (review) should be detected by this test.

Update: The test is now part of make static-test.

Issues/PRs references

#15957, #15855

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: build system Area: Build system Area: tests Area: tests and testing framework Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 9, 2021
@maribu maribu mentioned this pull request Feb 9, 2021
Merged
@maribu
Copy link
Member Author

maribu commented Feb 9, 2021

Static test is failing as expected. May I squash?

@maribu maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 9, 2021
fjmolinas
fjmolinas reviewed Feb 9, 2021
View reviewed changes
dist/tools/ci/static_tests.sh Outdated
run ./dist/tools/flake8/check.sh
run ./dist/tools/headerguards/check.sh
run ./dist/tools/buildsystem_sanity_check/check.sh
run ./tests/feature_resolution/check.sh
Copy link
Contributor

@fjmolinas fjmolinas Feb 9, 2021

Choose a reason for hiding this comment

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

Wouldn't this fit better in dist/tools/...?

fjmolinas
fjmolinas reviewed Feb 9, 2021
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Nice and needed addition. Maybe these files fit better under makefiles/tests?

@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 9, 2021

Can we add this test case as well

# inputs
FEATURES_OPTIONAL := a
FEATURES_REQUIRED :=
FEATURES_REQUIRED_ANY := d|b|a
FEATURES_PROVIDED := a b c
FEATURES_BLACKLIST :=
FEATURES_CONFLICT :=

# expected results
EXPECTED_FEATURES_USED := a
EXPECTED_FEATURES_MISSING :=
EXPECTED_FEATURES_USED_BLACKLISTED :=
EXPECTED_FEATURES_CONFLICTING :=

include Makefile.test

@maribu
makefiles/features_check.inc.mk: fix declaration order
8fb3066 
FEATURES_CONFLICTING previously was declared prior to the function it is
calling, resulting in empty output during the first dependency resolution
iteration. This fixes the order so that the conflicting features are detected
right from the first recursion.
@maribu
build system: fix using optional blacklisted features
4304600
@maribu maribu force-pushed the feature-resolution-test branch from c8f28c2 to 09940d9 Compare February 10, 2021 08:43
@maribu maribu requested a review from jia200x as a code owner February 10, 2021 08:43
@maribu
Copy link
Member Author

maribu commented Feb 10, 2021

Another bug uncovered: blacklisted features end still up being used if they are optional.

@maribu
tests/feature_resolution: add build system unit tests
ed2ae85 
This test "application" contains a set of unit tests for the feature resolution
of RIOT's build system.
@maribu maribu force-pushed the feature-resolution-test branch from 09940d9 to ed2ae85 Compare February 10, 2021 20:05
@maribu
Copy link
Member Author

maribu commented Feb 12, 2021

@fjmolinas: Ping?

@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 12, 2021

Lets spin up the ci

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 12, 2021
@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 12, 2021

I'm running the dependency check, although I expect differences since this PR caught some bugs, Probably will have some bugs in gnrc_minimal since without this PR both libc get selected.

@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 12, 2021

But

I'm running the dependency check, although I expect differences since this PR caught some bugs, Probably will have some bugs in gnrc_minimal since without this PR both libc get selected.

So I just want to take a look but won't be a blocking point since this PR provides tests.

@fjmolinas fjmolinas mentioned this pull request Feb 12, 2021
Closed
@kfessel kfessel mentioned this pull request Feb 12, 2021
Open
fjmolinas
fjmolinas approved these changes Feb 13, 2021
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Everything seems ok, lets go, hopefully, there are no more hidden bugs lingering.

@fjmolinas fjmolinas merged commit 579de7c into RIOT-OS:master Feb 13, 2021
@maribu
Copy link
Member Author

maribu commented Feb 13, 2021

Thanks!

@maribu maribu deleted the feature-resolution-test branch February 13, 2021 09:11
@kfessel kfessel mentioned this pull request Feb 15, 2021
Merged
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@smlng smlng Awaiting requested review from smlng

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

Assignees

No one assigned

Labels

Area: build system Area: Build system 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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2021.04

Development

Successfully merging this pull request may close these issues.

3 participants

@maribu @fjmolinas @kaspar030