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

Unable to select LOG_DICTIONARY_SUPPORT when TEST_LOGGING_DEFAULTS=y #34696

Closed
yashi opened this issue Apr 29, 2021 · 5 comments · Fixed by #34883
Closed

Unable to select LOG_DICTIONARY_SUPPORT when TEST_LOGGING_DEFAULTS=y #34696

yashi opened this issue Apr 29, 2021 · 5 comments · Fixed by #34883
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Milestone

Comments

@yashi
Copy link
Collaborator

yashi commented Apr 29, 2021

Describe the bug
The commit 7f08061 changed imply LOG_MINIMAL in subsys/testsuite/Kconfig to select LOG_MINIMAL, which prohibits menuconfig users not to select desired logging mode and effectively reverts the commit 34b7a6c.

Because it selects LOG_MINIMAL instead of LOG_MODE_MINIMAL, you can't test any feature such as LOG_DICTIONARY_SUPPORT if you enable TEST_LOGGING_DEFAULTS.

BTW, as you may already know, you can't imply LOG_MODE_MINIMAL.

warning: the choice symbol LOG_MODE_MINIMAL (defined at subsys/logging/Kconfig.mode:43) is implied by the following symbols, but select/imply has no effect on choice symbols
 - TEST_LOGGING_DEFAULTS (defined at subsys/testsuite/Kconfig:78)

error: Aborting due to Kconfig warnings

All I can come up with is to default LOG_MODE_MINIMAL if TEST_LOGGING_DEFAULTS on LOG_MODE as described in #6948.

To Reproduce

$ west build -b qemu_cortex_m3 -t menuconfig zephyr/tests/kernel/spinlock/

Expected behavior
I'd like to select LOG_DICTIONARY_SUPPORT when TEST_LOGGING_DEFAULTS=y

Impact
Unable to test LOG_DICTIONARY_SUPPORT with ztest.

Environment (please complete the following information):

  • OS: Linux
  • Toolchain: SDK
  • Commit SHA: zephyr-v2.5.0-2856-ge6248db31d
@yashi yashi added the bug The issue is a bug, or the PR is fixing a bug label Apr 29, 2021
@yashi
Copy link
Collaborator Author

yashi commented Apr 29, 2021

I guess I misread the help string on TEST_LOGGING_DEFAULTS. It seems like when you want the minimal mode logging with test, you select TEST_LOGGING_DEFAULTS, which is y by default. If you want another logging mode, you need to deselect it.

hmm... Why do we need this TEST_LOGGING_DEFAULTS?

@carlescufi
Copy link
Member

I think that TEST_LOGGING_DEFAULTS was introduced by Andrew but @nashif knows why it's necessary.

@nordic-krch
Copy link
Contributor

@yashi, TEST_LOGGING_DEFAULTS was introduce to enforce using minimal logging as the default mode in the tests. Log minimal is a simple redirection to printk so it does not support LOG_DICTIONARY_SUPPORT,

@yashi
Copy link
Collaborator Author

yashi commented Apr 30, 2021

@nordic-krch Thanks for your feedback. Do we really want to enfoce minimal logging with tests? The commit 34b7a6c reads:

The testsuite was always forcing minimal logging. This is problematic as it does not allow user to see full logging string. Allow user to override the minimal logging if needed, the default is still to enable minimal logging.

I'm not sure what was the intent. If we just want minimal logging as the default with test suite, we can do like this.

diff --git a/subsys/logging/Kconfig.mode b/subsys/logging/Kconfig.mode
index eee352c369..d16647791b 100644
--- a/subsys/logging/Kconfig.mode
+++ b/subsys/logging/Kconfig.mode
@@ -3,6 +3,7 @@
 
 choice LOG_MODE
 	prompt "Mode"
+	default LOG_MODE_MINIMAL if TEST
 	default LOG_MODE_DEFERRED
 
 config LOG_MODE_DEFERRED
diff --git a/subsys/testsuite/Kconfig b/subsys/testsuite/Kconfig
index cb8a3a21dc..b871bb27ee 100644
--- a/subsys/testsuite/Kconfig
+++ b/subsys/testsuite/Kconfig
@@ -75,18 +75,6 @@ config TEST_USERSPACE
 	  pass, CONFIG_ARCH_HAS_USERSPACE should be filtered in its
 	  testcase.yaml.
 
-config TEST_LOGGING_DEFAULTS
-	bool "Enable test case logging defaults"
-	depends on TEST
-	select LOG
-	select LOG_MINIMAL
-	default y
-	help
-	  Option which implements default policy of enabling logging in
-	  minimal mode for all test cases. For tests that need alternate
-	  logging configuration, or no logging at all, disable this
-	  in the project-level defconfig.
-
 config TEST_ENABLE_USERSPACE
 	bool
 	depends on TEST_USERSPACE

Many prj.confs and defconfigs set TEST_LOGGING_DEFAULTS=n, so we have to make sure that the change doesn't break, but I hope you can see my idea.

Anyway, if we don't support anything but minimal logging with the test suite, you can close this issue. Thanks!

@nashif nashif assigned dcpleung and unassigned nashif, dcpleung and nordic-krch May 4, 2021
dcpleung pushed a commit to dcpleung/zephyr that referenced this issue May 5, 2021
The testsuite was always forcing minimal logging. This is problematic
as it does not allow user to see full logging string. Allow user to
override the minimal logging if needed, the default is still to
enable minimal logging.

[DL: Commit 7f08061 reverts this.
     Since this is useful, let's re-apply this.]

Fixes zephyrproject-rtos#34696

Signed-off-by: Jukka Rissanen <[email protected]>
Signed-off-by: Daniel Leung <[email protected]>
@dcpleung
Copy link
Member

dcpleung commented May 5, 2021

Note that the testsuite defaults to using printk() for TC_PRINT(), and also a bunch of tests are actually using printk(), so you will need to enable CONFIG_LOG_PRINTK.

dcpleung pushed a commit to dcpleung/zephyr that referenced this issue May 6, 2021
The testsuite was always forcing minimal logging. This is problematic
as it does not allow user to see full logging string. Allow user to
override the minimal logging if needed, the default is still to
enable minimal logging.

[DL: Commit 7f08061 reverts this.
     Since this is useful, let's re-apply this.]

Fixes zephyrproject-rtos#34696

Signed-off-by: Jukka Rissanen <[email protected]>
Signed-off-by: Daniel Leung <[email protected]>
@galak galak added this to the v2.6.0 milestone May 11, 2021
@galak galak added the priority: low Low impact/importance bug label May 11, 2021
nashif pushed a commit that referenced this issue May 17, 2021
The testsuite was always forcing minimal logging. This is problematic
as it does not allow user to see full logging string. Allow user to
override the minimal logging if needed, the default is still to
enable minimal logging.

[DL: Commit 7f08061 reverts this.
     Since this is useful, let's re-apply this.]

Fixes #34696

Signed-off-by: Jukka Rissanen <[email protected]>
Signed-off-by: Daniel Leung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants