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

tests: Extend test_extension_command_duplicate #780

Merged

Conversation

57300
Copy link
Contributor

@57300 57300 commented Jan 21, 2025

This test is meant to cover the handling of duplicate command names, including the warnings to be emitted. Previously, it only tested the case where two extension commands had the same name. The other case is duplicating a built-in command name, which prompts a different warning message, so update the test to include it.

Additionally, ensure that each duplicate command in the test comes from a different project, so that each warning message is expected to contain a different project name. This serves to verify the bug fix from commit edd1cee.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Nice, thanks. I'm curious how you noticed this? Were you using a west version older than the fix?

tests/test_project.py Outdated Show resolved Hide resolved
# will print a warning.
# West should disregard an extension command if its name is already taken,
# either by a built-in command or another extension command added earlier.
# A warning should also appear for each such case.
Copy link
Collaborator

Choose a reason for hiding this comment

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

pydoc syntax instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean: move this comment above the function, or make it a docstring?
So far, I just kept it consistent with other comments in this particular file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant pydoc. It's placed exactly like a pydoc but just missing the syntax.

I didn't see that all other tests are like this, sorry. I can't see a good reason why. I think starting to use the pydoc syntax would look consistent enough while making progress at the same time.

pydocs are automatically extracted by very many tools including python itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see a good reason why.

I tried to write pydoc for APIs and comments for things that weren't API, as a pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @mbolivar , now I see https://docs.zephyrproject.org/latest/develop/west/west-apis.html is extracted from the source. Is that automated somewhere or manual?

I tried to write pydoc for APIs and comments for things that weren't API, as a pattern.

Makes sense but the tests/ are located in an entirely different, top-level directory so they should not interfere.

Either way this should really be written down somewhere.

Apologies @57300 for the off-topic discussion, feel free to ignore.

@57300
Copy link
Contributor Author

57300 commented Jan 22, 2025

Nice, thanks. I'm curious how you noticed this? Were you using a west version older than the fix?

Yes, I was on an older version. To be honest, I noticed the bug while looking for a crude way to override west flash (before out-of-tree runner support was added to Zephyr).

This test is meant to cover the handling of duplicate command names,
including the warnings to be emitted. Previously, it only tested the
case where two extension commands had the same name. The other case is
duplicating a built-in command name, which prompts a different warning
message, so update the test to include it.

Additionally, ensure that each duplicate command in the test comes from
a different project, so that each warning message is expected to contain
a different project name. This serves to verify the bug fix from commit
edd1cee.

Signed-off-by: Grzegorz Swiderski <[email protected]>
@57300 57300 force-pushed the test-extension-command-duplicate branch from 497bf3f to 1349b81 Compare January 31, 2025 12:34
@pdgendt pdgendt merged commit b75b86b into zephyrproject-rtos:main Jan 31, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants