[TA] Expose ActionName and enable multiple actions from same type

[ZulaMostafa]

This PR is for exposing ActionName again as it was disabled in the issue #22311. It also includes enabling multi actions from the same type as the service supports that right now. Resolves both #24441 and #24430.

I have some concerns regarding this issue:

• How should I deal with the breaking change that happened because of adding a new parameter to existing functions? for now I built the project with adding /p:BaselineAllAPICompatError=true. I am suggesting that we make overloaded functions for them, with validating that all actions from the same type have unique names.
• How should the tests be maintained? Should I add a test for every type of action? I am welcoming any suggestions for that.

[kinelski]

How should I deal with the breaking change that happened because of adding a new parameter to existing functions?

This happens very frequently in the Model Factory as we add new properties to models, as you can see here:

azure-sdk-for-net/sdk/textanalytics/Azure.AI.TextAnalytics/src/TextAnalyticsModelFactory.cs

Lines 263 to 290 in 1c0fb6f

	/// <summary>
	/// Initializes a new instance of <see cref="TextAnalytics.CategorizedEntity"/> for mocking purposes.
	/// </summary>
	/// <param name="text">Sets the <see cref="CategorizedEntity.Text"/> property.</param>
	/// <param name="category">Sets the <see cref="CategorizedEntity.Category"/> property.</param>
	/// <param name="subCategory">Sets the <see cref="CategorizedEntity.SubCategory"/> property.</param>
	/// <param name="score">Sets the <see cref="CategorizedEntity.ConfidenceScore"/> property.</param>
	/// <returns>A new instance of <see cref="TextAnalytics.CategorizedEntity"/> for mocking purposes.</returns>
	[EditorBrowsable(EditorBrowsableState.Never)]
	public static CategorizedEntity CategorizedEntity(string text, string category, string subCategory, double score)
	{
	return new CategorizedEntity(new Entity(text, category, subCategory, default, default, score));
	}
	/// <summary>
	/// Initializes a new instance of <see cref="TextAnalytics.CategorizedEntity"/> for mocking purposes.
	/// </summary>
	/// <param name="text">Sets the <see cref="CategorizedEntity.Text"/> property.</param>
	/// <param name="category">Sets the <see cref="CategorizedEntity.Category"/> property.</param>
	/// <param name="subCategory">Sets the <see cref="CategorizedEntity.SubCategory"/> property.</param>
	/// <param name="score">Sets the <see cref="CategorizedEntity.ConfidenceScore"/> property.</param>
	/// <param name="offset">Sets the <see cref="CategorizedEntity.Offset"/> property.</param>
	/// <param name="length">Sets the <see cref="CategorizedEntity.Length"/> property.</param>
	/// <returns>A new instance of <see cref="TextAnalytics.CategorizedEntity"/> for mocking purposes.</returns>
	public static CategorizedEntity CategorizedEntity(string text, string category, string subCategory, double score, int offset, int length)
	{
	return new CategorizedEntity(new Entity(text, category, subCategory, offset, length, score));
	}

In these situations we create a new overload as you suggested, and add the [EditorBrowsable(EditorBrowsableState.Never)] attribute to the old method so it's not suggested by IntelliSense anymore, but it can still be used by customers.

[kristapratico]

Maybe update the changelog with the new feature

[deyaaeldeen]

Implementation looks good!

[ZulaMostafa]

Implementation looks good!

Thanks!

[ZulaMostafa]

I rebased the branch and there are no conflicts now.
I've also added a single test for every action to make sure they can run multiple actions, and also to make sure that actionName gets sent and received correctly. I was also thinking about adding a test with all the actions included but with multiple ones for each of them, what do you think?

[jsquire]

LGTM; failures seem to indicate that the recordings need to be redone to accommodate the API version change.

[ZulaMostafa]

LGTM; failures seem to indicate that the recordings need to be redone to accommodate the API version change.

Should we make a separate PR to run the tests? I noticed that only 4 of them needs to be redone.

[maririos]

This file shouldn't be needed anymore now that you added the overloads to the TextAnalyticsModelFactory class, right?

[maririos]

LGTM; failures seem to indicate that the recordings need to be redone to accommodate the API version change.

Should we make a separate PR to run the tests? I noticed that only 4 of them need to be redone.

I think the general idea is:

• If you removed tests that had recordings, make sure the recordings are deleted (I don't think that is the case, but worth checking)
• If you added LiveTest (which you did for every action to verify the multiple action implementation) there should be 2 recording files associated with the test (sync and async) and it looks like these were not generated/added

[ZulaMostafa]

I added a new test AnalyzeOperationWithMultipleActionsOfSameType to make sure that it works well if we have multiple actions and each of them have multiple one. Whenever I run it I get an internal server error, failed to register new job.
any idea why @maririos ?

[jsquire]

Should we make a separate PR to run the tests? I noticed that only 4 of them needs to be redone.

We'll want to make sure that the tests pass as part of this work; we don't want to introduce changes into the repository that break test runs.