Skip to content

Conversation

@josepmariapujol-unity
Copy link
Collaborator

@josepmariapujol-unity josepmariapujol-unity commented Jan 12, 2026

Description

Recently discovered some warnings after regenerating CI from another PR.
Warnings:
Screenshot 2026-01-12 at 11 11 51

Testing status & QA

Regenerating ci after changes no longer display warnings.

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

  • Complexity: 1
  • Halo Effect: 1

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@u-pr-agent
Copy link
Contributor

u-pr-agent bot commented Jan 12, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

Small, localized changes, but behavior shifts (null-handling and stricter enum parsing) warrant a quick validation pass across CI config permutations.
🏅 Score: 84

Changes improve robustness and error clarity, but new failure modes may surface if CI JSON contains missing/empty enum fields or if `GetFields()` includes non-enum members.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Enum parsing

GetEnumValue iterates type.GetFields() which can include non-value fields; ensure only actual enum members are considered (e.g., filter by field.IsLiteral) and confirm EnumMemberAttribute is consistently present for all expected values.

public static TEnum GetEnumValue<TEnum>(string value) where TEnum : Enum
{
    var type = typeof(TEnum);
    foreach (var field in type.GetFields())
    {
        var attribute = field.GetCustomAttribute<EnumMemberAttribute>();

        // Ensure both the attribute and its value match
        if (attribute?.Value == value)
        {
            var val = field.GetValue(null);
            if (val is TEnum result)
            {
                return result;
            }
        }
    }

    throw new ArgumentException($"Value '{value}' not found in {type.Name}.");
New hard-fail

JSON lookups now default missing fields to empty strings, which will cause GetEnumValue to throw; verify config always provides flavor/type for both build and run, or consider a safer fallback/error message that identifies the platform/key path.

MobileBuildPlatforms.Add(platform, new Platform(
    new Agent(v?["build"]?["image"]?.ToString() ?? string.Empty, 
        Utilities.GetEnumValue<FlavorType>(v?["build"]?["flavor"]?.ToString() ?? string.Empty), 
        Utilities.GetEnumValue<ResourceType>(v?["build"]?["type"]?.ToString() ?? string.Empty)),
    platform));

MobileTestPlatforms.Add(platform, new Platform(
    new Agent(v?["run"]?["image"]?.ToString() ?? string.Empty, 
        Utilities.GetEnumValue<FlavorType>(v?["run"]?["flavor"]?.ToString() ?? string.Empty), 
        Utilities.GetEnumValue<ResourceType>(v?["run"]?["type"]?.ToString() ?? string.Empty),
        v?["run"]?["model"]?.ToString()),
    platform));
Condition match

The BuildJobs detection now depends on job.Name being non-null; confirm that null names should be treated as non-build jobs, and consider whether the match should be more specific (case-sensitivity/substring collisions) for CI stability.

if (job.Name != null && job.Name.Contains("BuildJobs"))
    break;
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@u-pr-agent
Copy link
Contributor

u-pr-agent bot commented Jan 12, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate required config fields first

Avoid passing string.Empty into enum parsing, since it converts missing/invalid JSON
into a hard-to-diagnose ArgumentException. Validate that required fields exist and
throw a targeted configuration error (or skip the platform) before calling
GetEnumValue.

Tools/CI/Settings/InputSystemSettings.cs [160-163]

-new Agent(v?["build"]?["image"]?.ToString() ?? string.Empty, 
-    Utilities.GetEnumValue<FlavorType>(v?["build"]?["flavor"]?.ToString() ?? string.Empty), 
-    Utilities.GetEnumValue<ResourceType>(v?["build"]?["type"]?.ToString() ?? string.Empty)),
+var build = v?["build"] as JObject ?? throw new InvalidDataException($"Missing 'build' section for '{k}'.");
+var buildImage = build["image"]?.ToString() ?? throw new InvalidDataException($"Missing build.image for '{k}'.");
+var buildFlavor = build["flavor"]?.ToString() ?? throw new InvalidDataException($"Missing build.flavor for '{k}'.");
+var buildType = build["type"]?.ToString() ?? throw new InvalidDataException($"Missing build.type for '{k}'.");
 
+new Agent(buildImage,
+    Utilities.GetEnumValue<FlavorType>(buildFlavor),
+    Utilities.GetEnumValue<ResourceType>(buildType)),
+
Suggestion importance[1-10]: 7

__

Why: Passing string.Empty into Utilities.GetEnumValue<TEnum>() will reliably throw ArgumentException when config keys are missing, but the error will be less actionable than a targeted config validation error. Adding explicit validation for required JSON fields in ReadMobileConfig() improves debuggability and avoids masking config issues.

Medium
Restrict to literal enum fields

GetFields() returns non-enum fields as well, and field.GetValue(null) can be null
(or not TEnum), which currently just falls through and hides the real mismatch.
Filter to literal enum fields and return via Enum.Parse/Enum.ToObject to guarantee a
correct TEnum when a match is found.

Tools/CI/Recipes/Utilities/Utilities.cs [58-65]

+if (!field.IsLiteral)
+    continue;
+
 if (attribute?.Value == value)
-{
-    var val = field.GetValue(null);
-    if (val is TEnum result)
-    {
-        return result;
-    }
-}
+    return (TEnum)Enum.ToObject(type, field.GetRawConstantValue());
Suggestion importance[1-10]: 6

__

Why: type.GetFields() includes non-literal fields (e.g., value__), so filtering with field.IsLiteral is a correctness/robustness improvement. Returning via Enum.ToObject is consistent with enum constant values, though the current val is TEnum result approach will typically work for literal fields anyway, so impact is moderate.

Low
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@codecov-github-com
Copy link

codecov-github-com bot commented Jan 12, 2026

Codecov Report

All modified and coverable lines are covered by tests ✅

@@             Coverage Diff             @@
##           develop    #2319      +/-   ##
===========================================
- Coverage    77.96%   77.95%   -0.01%     
===========================================
  Files          476      476              
  Lines        97443    97443              
===========================================
- Hits         75967    75961       -6     
- Misses       21476    21482       +6     
Flag Coverage Δ
inputsystem_MacOS_2022.3_project 75.47% <ø> (ø)
inputsystem_MacOS_6000.0 5.32% <ø> (ø)
inputsystem_MacOS_6000.0_project 77.37% <ø> (+<0.01%) ⬆️
inputsystem_MacOS_6000.3 5.32% <ø> (ø)
inputsystem_MacOS_6000.3_project 77.36% <ø> (-0.01%) ⬇️
inputsystem_MacOS_6000.4 5.32% <ø> (ø)
inputsystem_MacOS_6000.4_project 77.37% <ø> (+<0.01%) ⬆️
inputsystem_MacOS_6000.5 5.32% <ø> (ø)
inputsystem_MacOS_6000.5_project 77.36% <ø> (+<0.01%) ⬆️
inputsystem_Ubuntu_2022.3 5.54% <ø> (ø)
inputsystem_Ubuntu_2022.3_project 75.26% <ø> (ø)
inputsystem_Ubuntu_6000.0 5.32% <ø> (ø)
inputsystem_Ubuntu_6000.0_project 77.16% <ø> (ø)
inputsystem_Ubuntu_6000.3 5.32% <ø> (ø)
inputsystem_Ubuntu_6000.3_project 77.17% <ø> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.4 5.33% <ø> (ø)
inputsystem_Ubuntu_6000.4_project 77.17% <ø> (-0.01%) ⬇️
inputsystem_Ubuntu_6000.5 5.33% <ø> (ø)
inputsystem_Ubuntu_6000.5_project 77.17% <ø> (-0.01%) ⬇️
inputsystem_Windows_2022.3 5.54% <ø> (ø)
inputsystem_Windows_2022.3_project 75.60% <ø> (-0.01%) ⬇️
inputsystem_Windows_6000.0 5.32% <ø> (ø)
inputsystem_Windows_6000.0_project 77.49% <ø> (-0.01%) ⬇️
inputsystem_Windows_6000.3 5.32% <ø> (ø)
inputsystem_Windows_6000.3_project 77.49% <ø> (ø)
inputsystem_Windows_6000.4 5.32% <ø> (ø)
inputsystem_Windows_6000.4_project 77.49% <ø> (+<0.01%) ⬆️
inputsystem_Windows_6000.5 5.32% <ø> (ø)
inputsystem_Windows_6000.5_project 77.49% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@josepmariapujol-unity josepmariapujol-unity merged commit 2d1677c into develop Jan 12, 2026
95 of 108 checks passed
@josepmariapujol-unity josepmariapujol-unity deleted the input/ci-warning-utilities-file branch January 12, 2026 15:36
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