Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Jul 23, 2025

Moves us down to a smaller and simpler set of testing overloads to maintain.

Note: there is likely more cleanup coming. The test harness has morphed so much, and there's lots of redundancy everywhere.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner July 23, 2025 07:56

[Fact]
public Task Test1()
=> TestInRegularAndScript1Async(
Copy link
Member Author

Choose a reason for hiding this comment

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

we had to have this '1' in the name to prevent ambiguity collisions. Now the duplicative members are gone. so the naming ludge isn't needed.

@@ -405,32 +405,15 @@ protected async Task TestActionCountAsync(
}

internal Task TestInRegularAndScriptAsync(
Copy link
Member Author

Choose a reason for hiding this comment

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

these (and other methods like these) were the core methods i wanted to remove. They're massive, and we often take them (with small tweaks) and put in other locations. Having these be trivial (input, expected, optional parameters) means getting rid of a bunch of overloads that confusing called each other (and often could do things like miss some parameter along the way).

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-ide ptal

Assert.Equal(count, actions.Length);
}

internal Task TestInRegularAndScriptAsync(
Copy link
Member

Choose a reason for hiding this comment

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

Delete?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you're asking me.

Copy link
Member

@tmat tmat Jul 23, 2025

Choose a reason for hiding this comment

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

Diff shows the code as commented out, so I'm checking if you meant to delete it.

@CyrusNajmabadi CyrusNajmabadi requested a review from tmat July 23, 2025 15:13
@CyrusNajmabadi CyrusNajmabadi merged commit a8ffa9a into dotnet:main Jul 23, 2025
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the testParameters branch July 23, 2025 15:52
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 23, 2025
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants