Adding L0 tests for Legacy NodeHandler logic | Adding 2 Knobs for feature flag & EOL toggle status value #5421

rishabhmalikMS · 2025-12-08T08:53:58Z

Context

This PR introduces comprehensive L0 test specifications for NodeHandler behavior to establish a test-driven development foundation for upcoming EOL (End of Life) Node.js version policy and unified node selection strategy improvements. The test specifications define expected behaviors for both current legacy implementation and future unified strategy implementation.

Also added 2 knobs, one for feature flag usage to switch between legacy and new Node selection process. Another, to read the EOL toggle status value injected by server when user changes task settings from org settings from ADO.

Related Work: This is the first PR in a multi-part effort to modernize Node.js version handling in Azure Pipelines Agent with proper EOL policy enforcement and custom node path support.

Workitems links

Description

Added comprehensive test infrastructure and specifications for NodeHandler with detailed test scenarios covering:

Test Infrastructure:

NodeHandlerTestBase.cs - Clean base class for NodeHandler test execution with environment setup/teardown
NodeHandlerL0.TestSpecifications.cs - Comprehensive test scenarios covering all Node.js versions and edge cases
NodeHandlerL0.AllSpecs.cs - Unified test runner executing all scenarios
NodeHandlerCollections.cs - Test collection configuration for proper isolation

Test Coverage Areas:
2. Node6 Scenarios - Legacy Node 6.x behavior and EOL policy interactions
3. Node10 Scenarios - Node 10.x behavior and EOL policy interactions
4. Node16 Scenarios - Node 16.x behavior and EOL policy interactions
5. Node20 Scenarios - Current default Node 20.x behavior
6. Node24 Scenarios - Latest Node 24.x support
7. Container Scenarios - Node selection in containerized environments
8. EOL Policy Scenarios - End-of-life version handling differences between legacy and unified strategies

Test Structure: Each test scenario includes:

Input configuration (handler type, environment knobs, glibc errors, container mode)
Strategy-specific expectations (legacy vs unified behavior)
Clear success/failure criteria and expected error messages
Please refer TestScenario Field Structure & Usage regarding our test specifications have been defined and used.

Details of each test scenario added/covered: Test scenarios

Risk Assessment (Low / Medium / High)

Low Risk - This PR contains only test code with no changes to production logic. The tests currently run against existing legacy NodeHandler implementation, with some CustomNode tests intentionally failing to demonstrate implementation gaps for future development.

Unit Tests Added or Updated (Yes / No)

Yes

Additional Testing Performed

List manual or automated tests performed beyond unit tests (e.g., integration, scenario, regression).

Change Behind Feature Flag (Yes / No)

Not applicable - This PR contains only test code with no production changes requiring feature flags.

Tech Design / Approach

Design has been written and reviewed.
Any architectural decisions, trade-offs, and alternatives are captured.

Documentation Changes Required (Yes/No)

Test-First Development Approach:

Comprehensive test specifications define behavior before implementation
Clear separation between legacy and unified strategy expectations
Strategy-agnostic test infrastructure supporting both approaches

Logging Added/Updated (Yes/No)

Appropriate log statements are added with meaningful messages.
Logging does not expose sensitive data.
Log levels are used correctly (e.g., info, warn, error).

Telemetry Added/Updated (Yes/No)

Custom telemetry (e.g., counters, timers, error tracking) is added as needed.
Events are tagged with proper metadata for filtering and analysis.
Telemetry is validated in staging or test environments.

Rollback Scenario and Process (Yes/No)

Rollback plan is documented.

Dependency Impact Assessed and Regression Tested (Yes/No)

All impacted internal modules, APIs, services, and third-party libraries are analyzed.
Results are reviewed and confirmed to not break existing functionality.

…ed node | L0 tests for node handlers

…quence | Preventing leakage of test specific environment settings for knobs between node handler tests (both new and legacy)

…checks from handlers | Adding string tempalates for errors and warning and removing hard-coded erros and warning from code

…implyfying unit test scenarios to avoid using redundant code | Update NodeHandlerTestBase to reflect the simplified test scenario usage for unning tests

… check inside getnodeLocation function) | Added custom node path L0 tests | Updated test base code to integrate custom node tests

rishabhmalikMS · 2025-12-08T09:35:44Z

/azp run

azure-pipelines · 2025-12-08T09:35:58Z

Azure Pipelines successfully started running 1 pipeline(s).

src/Agent.Sdk/Knob/AgentKnobs.cs

src/Test/L0/NodeHandlerL0.cs

sanjuyadav24

Please share a table with all cases mentioned in test cases to validate if any case is not missed and expected behavior in each scenario

sanjuyadav24 · 2025-12-08T10:48:16Z

src/Test/L0/NodeHandlerL0.TestSpecifications.cs

+            ),
+
+            new TestScenario(
+                name: "Node6_DefaultBehavior_EOLPolicyDisabled",


whats the difference between above and this test case?

Essentially these are similar scenarios, for default behavior for node 6 when EOL knob is set to false or the knob itself not being available (this however won't happen as we have default value present in agent knobs). Test added just to cover all basis.

sanjuyadav24 · 2025-12-08T10:49:14Z

src/Test/L0/NodeHandlerL0.TestSpecifications.cs

+
+            new TestScenario(
+                name: "Node6_WithGlobalUseNode10Knob",
+                description: "Node6 handler with global Node10 knob: legacy uses Node10, unified ignores deprecated knob and uses Node6",


why node10 doesn't overwrite node6 knob?

AGENT_USE_NODE10 overrides to node 10 in older implementation. In new implementation, as discussed in design discussion(s), we are getting away from using this knob in new approach.

…com/microsoft/azure-pipelines-agent into users/rishabhmalikMS/EOLnodeL0Tests

tarunramsinghani · 2025-12-09T10:11:28Z

Wrong PR label, use internal instead of enhancement.

rishabhmalikMS · 2025-12-10T04:49:36Z

Wrong PR label, use internal instead of enhancement.

Updated

…ndler tests - Replace useUnifiedStrategy → useStrategy parameter - Replace unifiedExpectedNode → strategyExpectedNode property - Replace unifiedExpectSuccess → strategyExpectSuccess property - Replace unifiedExpectedError → strategyExpectedError property - Replace shouldMatchBetweenModes → shouldMatchBetweenLegacyAndStrategy property - Update environment variable AGENT_USE_UNIFIED_NODE_STRATEGY → AGENT_USE_NODE_STRATEGY - Update test descriptions to use "strategy-based" instead of "unified" - Update TestScenario class properties and constructor parameters This improves code clarity by using terminology that directly relates to the strategy pattern implementation rather than the generic term "unified".

rishabhmalikMS · 2025-12-11T07:16:09Z

/azp run

azure-pipelines · 2025-12-11T07:16:23Z

Azure Pipelines successfully started running 1 pipeline(s).

MantavyaDh · 2025-12-12T08:25:52Z

src/Test/L0/NodeHandlerTestBase.cs

+                    if (expectations.ExpectSuccess)
+                    {
+                        string actualLocation = nodeHandler.GetNodeLocation(
+                            node20ResultsInGlibCError: scenario.Node20GlibcError,
+                            node24ResultsInGlibCError: scenario.Node24GlibcError,
+                            inContainer: scenario.InContainer);
+
+                        string expectedLocation = GetExpectedNodeLocation(expectations.ExpectedNode, scenario, thc);
+                        Assert.Equal(expectedLocation, actualLocation);
+                    }


What does ExpectSuccess mean? We can assert the output of GetNodeLocation with our ExpectedNodeLocation, to handle failure of the function GetNodeLocation we can maybe use a try catch.

This is removed as per our discussion on below comment

MantavyaDh · 2025-12-12T08:38:34Z

src/Test/L0/NodeHandlerTestBase.cs

+                        var exception = Assert.Throws(scenario.ExpectedErrorType ?? typeof(FileNotFoundException),
+                            () => nodeHandler.GetNodeLocation(
+                                node20ResultsInGlibCError: scenario.Node20GlibcError,
+                                node24ResultsInGlibCError: scenario.Node24GlibcError,
+                                inContainer: scenario.InContainer));
+
+                        if (!string.IsNullOrEmpty(expectations.ExpectedError))
+                        {
+                            Assert.Contains(expectations.ExpectedError, exception.Message);
+                        }


If we are expecting an Exception to be thrown by function, why don't we catch the exception and there we check if the exception was expected, in this way we will not have to call the function GetNodeLocation twice.

This is updated to use try-catch approach and reduce fields from test scenario simplifying them further

MantavyaDh · 2025-12-12T08:48:06Z

src/Agent.Sdk/Knob/AgentKnobs.cs


+        public static readonly Knob EnableEOLNodeVersionPolicy = new Knob(
+            nameof(EnableEOLNodeVersionPolicy),
+            "When enabled, automatically upgrades tasks using end-of-life Node.js versions (6, 10, 16) to supported versions (Node 20.1 or Node 24). Throws error if no supported versions are available on the agent.",


This is slightly confusing, we are not upgrading tasks, rather when flag is enabled we disregard the EOL Node versions a task have.

This is updated to be more descriptive

raujaiswal · 2025-12-12T09:51:18Z

src/Test/L0/NodeHandlerL0.TestSpecifications.cs

+                name: "Node6_EOLPolicyEnabled_UpgradesToNode24",
+                description: "Node6 handler with EOL policy: legacy allows Node6, strategy-based upgrades to Node24",
+                handlerData: typeof(NodeHandlerData),
+                knobs: new() { ["AGENT_ENABLE_EOL_NODE_VERSION_POLICY"] = "true" },


I think after Sayantan suggestion, we have changed to AGENT_RESTRICT_EOL_NODE_VERSIONS in Agent Knob, but forget to change here and multiple places, could you please check if we want to update here as well.

This is updated

sanjuyadav24 · 2025-12-12T09:26:25Z

src/Test/L0/NodeHandlerL0.TestSpecifications.cs

+                legacyExpectedNode: "node10",
+                legacyExpectSuccess: true,
+                strategyExpectedNode: "node",
+                strategyExpectSuccess: true,


why this should use node6?

Because in new implementation node 10 (useNode10) knob is deprecated.

sanjuyadav24 · 2025-12-12T10:42:22Z

src/Test/L0/NodeHandlerTestBase.cs

+                            inContainer: scenario.InContainer);
+
+                        string expectedLocation = GetExpectedNodeLocation(expectations.ExpectedNode, scenario, thc);
+                        Assert.Equal(expectedLocation, actualLocation);


in the scenario we had two values, legacyexpected value and strategy expected value, here we are checking only one
is this expected?

Yes, right now this is expected. Because the tests are running only on existing original code of getNodeLocation. With our strategy code integration both node expectation will be included here.

…fields | Using Trt - Catch intest rnner code instead of this to reduce complexity | updated agnet knob text || Updated knob description to be more meaningful

rishabhmalikMS · 2025-12-12T11:03:51Z

/azp run

azure-pipelines · 2025-12-12T11:04:05Z

Azure Pipelines successfully started running 1 pipeline(s).

rishabhmalikMS added 14 commits December 3, 2025 09:46

Initial commit | node handler strategies | orchestrator to get select…

df2b8fc

…ed node | L0 tests for node handlers

Adding collection definition for unning all node handling tests in se…

42d4769

…quence | Preventing leakage of test specific environment settings for knobs between node handler tests (both new and legacy)

Adding selectedNode field in UnifiedNodeContext | Removing duplicate …

a8c7794

…checks from handlers | Adding string tempalates for errors and warning and removing hard-coded erros and warning from code

Adding equivalence and diverging test for legacy and new approach | S…

32becf6

…implyfying unit test scenarios to avoid using redundant code | Update NodeHandlerTestBase to reflect the simplified test scenario usage for unning tests

Node Handler integrtion fix for custom node (moved legacy custom node…

2e9ef63

… check inside getnodeLocation function) | Added custom node path L0 tests | Updated test base code to integrate custom node tests

Merged custom node scnario tests with all other test specs

6b5cc04

Code cleaning

244ebe6

Test scenarios updates

ce9dd01

Minor fixes

b2450ba

Adding CustomNodeHandlerData handler

b1a6dd7

minor fixes

af9b999

L0 unit tests for node handler with all scenarios for node handlers

a95c2ab

minor fix

84e3eb1

Code cleaning

0948f72

rishabhmalikMS added the enhancement label Dec 8, 2025

Merge branch 'master' into users/rishabhmalikMS/EOLnodeL0Tests

ac6b0cf

rishabhmalikMS changed the title ~~Users/rishabhmalik ms/eo lnode l0 tests~~ Adding L0 tests for Legacy NodeHandler logic | Adding 2 Knobs for feature flag & EOL toggle status value Dec 8, 2025

rishabhmalikMS marked this pull request as ready for review December 8, 2025 09:52

rishabhmalikMS requested review from a team as code owners December 8, 2025 09:52

dassayantan24 reviewed Dec 8, 2025

View reviewed changes

src/Agent.Sdk/Knob/AgentKnobs.cs Outdated Show resolved Hide resolved

dassayantan24 reviewed Dec 8, 2025

View reviewed changes

src/Test/L0/NodeHandlerL0.cs Show resolved Hide resolved

rishabhmalikMS mentioned this pull request Dec 8, 2025

Add Node.js version selection strategy framework with EOL policy support #5422

Open

sanjuyadav24 reviewed Dec 8, 2025

View reviewed changes

rishabhmalikMS added 2 commits December 9, 2025 09:25

Updating knob name to be consistent with server side updates.

b42cd15

Merge branch 'users/rishabhmalikMS/EOLnodeL0Tests' of https://github.…

d3f8dfa

…com/microsoft/azure-pipelines-agent into users/rishabhmalikMS/EOLnodeL0Tests

rishabhmalikMS removed the enhancement label Dec 9, 2025

rishabhmalikMS added the internal label Dec 9, 2025

rishabhmalikMS mentioned this pull request Dec 10, 2025

Node handler integration with Node Version Orchestrator #5425

Closed

MantavyaDh reviewed Dec 12, 2025

View reviewed changes

raujaiswal reviewed Dec 12, 2025

View reviewed changes

sanjuyadav24 reviewed Dec 12, 2025

View reviewed changes

rishabhmalikMS added 2 commits December 12, 2025 16:31

Removed strategyExpectSuccess, legacyExpectSuccess and ExpectSuccess …

5a08d24

…fields | Using Trt - Catch intest rnner code instead of this to reduce complexity | updated agnet knob text || Updated knob description to be more meaningful

Merge branch 'master' into users/rishabhmalikMS/EOLnodeL0Tests

2c73a58

Adding L0 tests for Legacy NodeHandler logic | Adding 2 Knobs for feature flag & EOL toggle status value #5421

Are you sure you want to change the base?

Adding L0 tests for Legacy NodeHandler logic | Adding 2 Knobs for feature flag & EOL toggle status value #5421

Conversation

rishabhmalikMS commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Description

Risk Assessment (Low / Medium / High)

Unit Tests Added or Updated (Yes / No)

Additional Testing Performed

Change Behind Feature Flag (Yes / No)

Tech Design / Approach

Documentation Changes Required (Yes/No)

Logging Added/Updated (Yes/No)

Telemetry Added/Updated (Yes/No)

Rollback Scenario and Process (Yes/No)

Dependency Impact Assessed and Regression Tested (Yes/No)

Uh oh!

rishabhmalikMS commented Dec 8, 2025

Uh oh!

azure-pipelines bot commented Dec 8, 2025

Uh oh!

Uh oh!

Uh oh!

sanjuyadav24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarunramsinghani commented Dec 9, 2025

Uh oh!

rishabhmalikMS commented Dec 10, 2025

Uh oh!

rishabhmalikMS commented Dec 11, 2025

Uh oh!

azure-pipelines bot commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raujaiswal Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishabhmalikMS commented Dec 12, 2025

Uh oh!

azure-pipelines bot commented Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rishabhmalikMS commented Dec 8, 2025 •

edited

Loading

raujaiswal Dec 12, 2025 •

edited

Loading