Adding L1 tests for host node selection #5456
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| else if (useStrategy) | ||
| { | ||
| // Strategy mode is allowed to fail during testing | ||
| Assert.True(actualResult == TaskResult.Failed, $"Strategy mode should either succeed or fail cleanly: {modeDescription}"); |
There was a problem hiding this comment.
should we have this test, what is the assertion
There was a problem hiding this comment.
this is removed
| hasNodeSelection = log.Any(x => x.Contains(NODE_SELECTION_LOG_PATTERN)); | ||
| } | ||
|
|
||
| Assert.True(hasNodeSelection, $"Should have node selection log: {modeDescription}"); |
There was a problem hiding this comment.
what are we asserting?
There was a problem hiding this comment.
why not check that if function is called? why do we have assertion based on log?
There was a problem hiding this comment.
We are asserting the actual node selection outcome from timeline logs here. Since we do not get any output from internal classes/utilities that run to decide node version to use. In L1 tests, we could only rely on timeline logs, hence this pattern matching is used.
| private const string NODE_SELECTION_LOG_PATTERN = "Using node path:"; | ||
| private const string NODE20_LOG_PATTERN = "node20"; | ||
|
|
||
| private void AssertTaskResult(TaskResult actualResult, bool useStrategy, string context = "") |
There was a problem hiding this comment.
whats the significance of this test?
There was a problem hiding this comment.
This AssertTaskResult function is removed.
| hasExpectedSelection = log.Any(x => x.Contains(NODE_SELECTION_LOG_PATTERN) && x.Contains(expectedPattern)); | ||
| } | ||
|
|
||
| Assert.True(hasExpectedSelection, $"Expected node selection '{expectedPattern}': {modeDescription}"); |
There was a problem hiding this comment.
shouldn't we check the final return value? why do we have test cases based on log statements?
There was a problem hiding this comment.
Node selection doesn't return accessible values in L1 tests - it affects internal state only visible through logs. Logs are the final result that users see. Unit tests are handling return value testing.
|
|
||
| if (results.Result == TaskResult.Succeeded) | ||
| { | ||
| if (expectedNodeFolder == NODE16_FOLDER) |
There was a problem hiding this comment.
here we are setting up the data, so why do we have if/else isn't we know what should be the expectedNodeFolder
There was a problem hiding this comment.
We donot get any output from internal classes/utilities that run to decide node version to use. In L1 tests, we could only rely on timeline logs, hence this pattern matching is used. This is similar to CoreL1Tests that we have in existing L1 test suite.
|
|
||
| AssertNodeSelectionAttempted(log, results.Result, useStrategy, "default behavior"); | ||
|
|
||
| if (results.Result == TaskResult.Succeeded) |
There was a problem hiding this comment.
if this is false then also test case will be successful, is that expected?
There was a problem hiding this comment.
This is removed to not have conditional assertions. All other places have been updated as well for this.
| Assert.True(results.Result == TaskResult.Succeeded || results.Result == TaskResult.Failed, | ||
| "Strategy mode should complete execution (success or failure expected)"); | ||
| } | ||
| else |
There was a problem hiding this comment.
shouldn't we have separate test cases for both legacy and strategy, instead of if/else as if/else test case might pass in some other unexpected scenario
There was a problem hiding this comment.
The idea was to use InlineData attribute to distinguish between legacy and strategy code flow. We do not have conditional assertions anymore. Only for distinguishing between legacy and strategy, the consditions are present,
|
|
||
| var steps = GetSteps(); | ||
| var taskStep = steps.FirstOrDefault(s => s.Name == "CmdLine"); | ||
| Assert.NotNull(taskStep); |
There was a problem hiding this comment.
How are we confirming that the execution is correct? Is it only a null check?
There was a problem hiding this comment.
In windows, it used powershell and node node, so here taskStep being not null is the only assertion
|
|
||
| if (actualResult == TaskResult.Succeeded) | ||
| { | ||
| // Both modes can succeed - this is fine |
There was a problem hiding this comment.
Should we add a log line for this case as well?
…ives - Remove AssertTaskResult method that incorrectly allowed strategy mode failures - Update both legacy and strategy modes to expect success with proper error handling - Simplify assertions by removing unnecessary TaskResult.Succeeded conditions - Make test validation straightforward without conditional logic weaknesses - Address PR feedback on log-based testing approach and assertion reliability Both modes now properly assert success, making tests meaningful validation of actual node selection behavior in L1 integration scenarios.
…soft/azure-pipelines-agent into users/rishabhmalik/L1Tests
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
4 participants
Context
This PR addresses L1 testing for Azure Pipelines Agent host-based node version selection. The existing test infrastructure was lacked to validate both strategy mode (new) and legacy mode node selection behaviors for Node.js task execution.
Description
NodeSelectionL1Tests.csto support both strategy mode and legacy mode validation with proper logging pattern detectionAGENT_USE_NODE_STRATEGY=true(strategy mode) andfalse(legacy mode) behaviorsRisk Assessment (Low)
Low Risk - This PR only adds and enhances test infrastructure without modifying production agent logic. The changes are isolated to the test suite (
src/Test/L1/Worker/) and improve validation coverage of existing node selection functionality. No runtime behavior changes for end users.Unit Tests Added or Updated (Yes / No)
Unit tests (L0 tests) were already added in previous PR.
Additional Testing Performed
List manual or automated tests performed beyond unit tests (e.g., integration, scenario, regression).
Change Behind Feature Flag (Yes / No)
No - These are test infrastructure improvements only.
Tech Design / Approach
Documentation Changes Required (Yes/No)
Indicate whether related documentation needs to be updated.
Logging Added/Updated (Yes/No)
Telemetry Added/Updated (Yes/No)
Rollback Scenario and Process (Yes/No)
Dependency Impact Assessed and Regression Tested (Yes/No)