Skip to content

Conversation

@achxkloel
Copy link
Collaborator

This MR fixes --include-all-apps flag in build command.
Now it's possible to include all apps to Junit report with:

idf-build-apps build --include-all-apps --junitxml junit.xml

Disabled apps are also shown in the output, with the correct reason taken from Manifest file.

Related

#102

@achxkloel achxkloel requested review from Copilot and hfudev November 25, 2025 14:42
Copilot finished reviewing on behalf of achxkloel November 25, 2025 14:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the --include-all-apps flag in the build command to properly include disabled apps in JUnit reports with their correct disable reasons from the Manifest file instead of a generic message.

Key changes:

  • Modified main.py to create separate FindArguments object when running build command to properly handle --include-all-apps flag
  • Removed generic "Build {status}. Skipping..." message that was overwriting the meaningful disable reasons set by manifest rules
  • Added console output to display disabled apps after build completion

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
idf_build_apps/main.py Creates separate FindArguments for build command to handle include_all_apps flag, adds disabled apps console output
idf_build_apps/app.py Removes line that overwrote manifest-provided disable reasons with generic message

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +407 to +410
# Because build needs to find apps, we need to create find_arguments here
find_arguments = FindArguments(
**{k: v for k, v in kwargs_without_none.items() if k in FindBuildArguments.model_fields}
)
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The new logic for creating FindArguments from BuildArguments kwargs lacks test coverage. This is a critical code path that ensures --include-all-apps works correctly with the build command by filtering kwargs to only those in FindBuildArguments.model_fields. Tests should verify that flags like --include-all-apps properly propagate from BuildArguments to FindArguments.

Copilot uses AI. Check for mistakes.
@achxkloel achxkloel force-pushed the fix/junit_disabled_apps branch 2 times, most recently from cbf08d4 to e5f1fca Compare November 25, 2025 15:34
@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
idf_build_apps
   __main__.py330%4–9
   app.py5498285%197, 242, 251–253, 285, 297, 353–354, 356, 365–366, 377–378, 438, 453, 491, 547–555, 565–566, 576, 594–595, 597, 613–622, 640–644, 659–662, 677–678, 696–697, 701–702, 713–715, 721, 734–736, 881–889, 899–929, 933–943, 1034–1040, 1043, 1065, 1083–1087
   args.py3713192%124, 188, 396–401, 411–416, 618, 621–622, 663, 686, 693–694, 706–708, 742–743, 871, 932, 934, 994, 1013–1023, 1042
   autocompletions.py292417%16–23, 31–54
   finder.py89496%154, 171–173
   log.py661183%33, 37, 48, 58–67, 112
   main.py2236870%73, 78–82, 129, 134–138, 177, 201–203, 207–208, 214–227, 241–244, 255–280, 370–377, 386–387, 418–425, 428, 434–435, 441–443, 516–531
   session_args.py53787%46–50, 56, 70
   utils.py1882388%26, 35, 113, 130–131, 155, 203, 246, 273–279, 292–295, 309–315, 397
idf_build_apps/junit
   report.py93990%82, 92, 109–111, 137, 144–145, 170
   utils.py291066%18, 26–35
idf_build_apps/manifest
   manifest.py2841395%121, 139, 147, 154, 159, 250, 307–312, 405, 435–436, 455, 507
   soc_header.py220%4–6
idf_build_apps/vendors
   pydantic_sources.py68593%52, 75, 78–81
TOTAL213629286% 

Tests Skipped Failures Errors Time
159 0 💤 0 ❌ 0 🔥 8m 56s ⏱️

@achxkloel achxkloel force-pushed the fix/junit_disabled_apps branch 2 times, most recently from 26d3efd to 9c473cd Compare November 28, 2025 10:21
@achxkloel achxkloel force-pushed the fix/junit_disabled_apps branch from 9c473cd to eeebc61 Compare November 28, 2025 10:24
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.

2 participants