Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Issue In current Versioning\Removed #736

Merged
merged 18 commits into from
Oct 31, 2024
Merged

Conversation

mcgallan
Copy link
Member

Fixed issue that exist in http\versioning\removed.
Related Issue Link: #734

Copy link

changeset-bot bot commented Sep 30, 2024

🦋 Changeset detected

Latest commit: 2d72382

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@azure-tools/cadl-ranch-specs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcgallan
Copy link
Member Author

mcgallan commented Oct 8, 2024

@tadelesh Regarding the mockapi added in Version\Removed tests, it does not actually achieve the expected effect. We want the mockapi to perform different validations and return different response bodies for the same operation based on different paths. However, in the code generated by autorest.csharp, the mockapi actually only validates based on the Expect Body of the first path and only returns the response body corresponding to the first path, without handling different paths differently. This has caused a bug. Now I want to know if there is a better solution for this situation. If the mockapi cannot perform corresponding validations and provide return values based on different paths, is there another way to achieve our expected effect?

@tadelesh
Copy link
Member

tadelesh commented Oct 9, 2024

@tadelesh Regarding the mockapi added in Version\Removed tests, it does not actually achieve the expected effect. We want the mockapi to perform different validations and return different response bodies for the same operation based on different paths. However, in the code generated by autorest.csharp, the mockapi actually only validates based on the Expect Body of the first path and only returns the response body corresponding to the first path, without handling different paths differently. This has caused a bug. Now I want to know if there is a better solution for this situation. If the mockapi cannot perform corresponding validations and provide return values based on different paths, is there another way to achieve our expected effect?

there are other test cases which also has multiple paths:

Scenarios.Resiliency_ServiceDriven_AddOptionalParam_fromNone = passOnSuccess(
. could you double check if it is a corner case or wrong usage?

@mcgallan
Copy link
Member Author

mcgallan commented Oct 9, 2024

@tadelesh Regarding the mockapi added in Version\Removed tests, it does not actually achieve the expected effect. We want the mockapi to perform different validations and return different response bodies for the same operation based on different paths. However, in the code generated by autorest.csharp, the mockapi actually only validates based on the Expect Body of the first path and only returns the response body corresponding to the first path, without handling different paths differently. This has caused a bug. Now I want to know if there is a better solution for this situation. If the mockapi cannot perform corresponding validations and provide return values based on different paths, is there another way to achieve our expected effect?

there are other test cases which also has multiple paths:

Scenarios.Resiliency_ServiceDriven_AddOptionalParam_fromNone = passOnSuccess(

. could you double check if it is a corner case or wrong usage?

This issue has been resolved. It was caused by using : instead of [:] to denote dynamic parameters in multiple paths. As for the resiliency part, since C# currently does not seem to support this feature, the successful execution of the test case does not indicate much. However, the way the mockapi is written is correct.

@mcgallan
Copy link
Member Author

@weidongxu-microsoft I have upgraded the PR code according to the new mockapi requirements and have also tested and verified it locally against three versions of the generated code. Please help review.

@mcgallan mcgallan merged commit 3317017 into Azure:main Oct 31, 2024
9 checks passed
v-hongli1 pushed a commit to v-hongli1/cadl-ranch that referenced this pull request Nov 1, 2024
* fix

* Update main.tsp

* update

* Update cadl-ranch-summary.md

* Update cadl-ranch-summary.md

* update

* Update mockapi.ts

* fix doc

* Update mockapi.ts

* Update mockapi.ts

* Update mockapi.ts
v-hongli1 pushed a commit to v-hongli1/cadl-ranch that referenced this pull request Nov 5, 2024
* fix

* Update main.tsp

* update

* Update cadl-ranch-summary.md

* Update cadl-ranch-summary.md

* update

* Update mockapi.ts

* fix doc

* Update mockapi.ts

* Update mockapi.ts

* Update mockapi.ts
v-hongli1 pushed a commit to v-hongli1/cadl-ranch that referenced this pull request Nov 5, 2024
* fix

* Update main.tsp

* update

* Update cadl-ranch-summary.md

* Update cadl-ranch-summary.md

* update

* Update mockapi.ts

* fix doc

* Update mockapi.ts

* Update mockapi.ts

* Update mockapi.ts
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