Skip to content

Add test macro MBED_TEST when building with mbed test #7627

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
Closed
wants to merge 1 commit into from
Closed

Add test macro MBED_TEST when building with mbed test #7627

wants to merge 1 commit into from

Conversation

deepikabhavnani
Copy link

Description

MBED_TEST macro can be used to guard test features or to select non-test related functionality (main) when building with mbed compile.

Related : #7626

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

CC @theotherjimmy @bridadan

@deepikabhavnani deepikabhavnani mentioned this pull request Jul 27, 2018
Closed
bridadan
bridadan approved these changes Jul 27, 2018
View reviewed changes
Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

Sweet!

tools/test.py Outdated
if options.macros is None:
options.macros = (['MBED_TEST'])
else:
options.macros.extend(['MBED_TEST'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I realize I mentioned extend in our offline talk, but you can also do options.macros.append('MBED_TEST') if you want to avoid creating another list.

Copy link
Author

Choose a reason for hiding this comment

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

Done

theotherjimmy
theotherjimmy reviewed Jul 27, 2018
View reviewed changes
tools/test.py Outdated
@@ -126,6 +126,12 @@
all_tests = {}
tests = {}

# Add the test macro, when compiling all test
if options.macros is None:
options.macros = (['MBED_TEST'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the ()?

@0xc0170 0xc0170 requested a review from bulislaw July 30, 2018 10:02
bridadan
bridadan previously requested changes Jul 30, 2018
View reviewed changes
tools/test.py Outdated
if options.macros is None:
options.macros = ['MBED_TEST']
else:
options.macros.append(['MBED_TEST'])
Copy link
Contributor

Choose a reason for hiding this comment

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

@deepikabhavnani this isn't right and will probably cause problems:

>>> a = [1, 2, 3]
>>> a.append([4])
>>> a
[1, 2, 3, [4]]

(Note the extra list)

It should be options.macros.append('MBED_TEST')

@cmonr cmonr dismissed bridadan’s stale review July 31, 2018 01:43

Comment addressed.

@0xc0170 0xc0170 requested a review from SenRamakri July 31, 2018 10:35
@0xc0170 0xc0170 requested a review from SeppoTakalo July 31, 2018 14:07
@SeppoTakalo
Copy link
Contributor

For what purposes is this used then?

@cmonr
Copy link
Contributor

cmonr commented Aug 14, 2018

@deepikabhavnani Question from @SeppoTakalo

@cmonr
Copy link
Contributor

cmonr commented Aug 14, 2018

@SeppoTakalo Does this PR answer your question?
#7629

@SeppoTakalo
Copy link
Contributor

@cmonr not really. I was trying to ask what particular problem does this fix?
To me, it looks like it is adding yet another layer of complexity, without any benefit.

/features/frameworks folder has been part of Mbed OS build since beginning. Why did it start to be problem now? And if its a problem, can it move to under TEST/ and allow things shared there.

I'm always against adding more build rules into our already complex collection of Python build scripts.

@deepikabhavnani
Copy link
Author

@SeppoTakalo - #7626 was the drive for the change, also I have added my points regarding why we should be having macro in comments.

And if its a problem, can it move to under TEST/ and allow things shared there.

This seems like good option, will like to hear from @ARMmbed/mbed-os-core @theotherjimmy @0xc0170 what they think of moving greentea-client, unity and utest to features/frameworks/TESTS.

@SeppoTakalo
Copy link
Contributor

I believe the is no common folder in TESTS structure that could be shared between all tests, but I would much rather see it there, than having separate build rules for mbed test compilation.

In future, we might even have more code that could be share between test cases.

@bridadan
Copy link
Contributor

@SeppoTakalo There is a common folder in tests structure. The folder structure is TESTS/COMMON. It was added in #5819. @deepikabhavnani Maybe placing the test frameworks in there would be a better option after all.

However, another use of this feature is to be able to let applications and tests live side-by-side. You could #ifdef around your application's main function like this:

#if !defined(MBED_TEST) || MBED_TEST == 0
int main() {
    // Application code
}
#endif

This would prevent the "multiple main" problem we currently have when building tests inside of an application

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theotherjimmy theotherjimmy theotherjimmy approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@bridadan bridadan bridadan left review comments

@bulislaw bulislaw Awaiting requested review from bulislaw

@SenRamakri SenRamakri Awaiting requested review from SenRamakri

@SeppoTakalo SeppoTakalo Awaiting requested review from SeppoTakalo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@deepikabhavnani @SeppoTakalo @cmonr @bridadan @theotherjimmy @0xc0170