-
Notifications
You must be signed in to change notification settings - Fork 26
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
chore(idf): possible renaming in idf #187
Conversation
@@ -52,7 +52,8 @@ def __init__( | |||
self.elf_file = self._get_elf_file() | |||
|
|||
# loadable elf file skip the rest of these | |||
if self.sdkconfig.get('APP_BUILD_TYPE_ELF_RAM'): | |||
# TODO to be improved in #186 | |||
if self.sdkconfig.get('APP_BUILD_TYPE_ELF_RAM') or self.sdkconfig.get('APP_BUILD_TYPE_RAM'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, can we set app.is_loadable_elf via pytest.mark.parametrize in the test case itself?
(just to reduce the amount of knowledge the test framework needs about the IDF internals)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, I prefer #186 instead of setting it explicitly, since users may forget to set it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, what is the point of #186? sdkconfig
file already contains all the options added by sdkconfig.rename, so if APP_BUILD_TYPE_ELF_RAM
got renamed into APP_BUILD_TYPE_ELF
, then sdkconfig
should contain both options.
Edit: maybe we have this issue because pytest-embedded-idf reads sdkconfig.json
, which unlike sdkconfig
doesn't contain the values added for backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. sorry I didn't explain #186 clearly. The reason that we're having this issue is like you said, the sdkconfig.json doesn't contain the history names.
No description provided.