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-special-union-within-anonymous-model-issue #2767

Merged

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Aug 26, 2024

fixes #2768
fixes #2772

@qiaozha qiaozha marked this pull request as ready for review August 26, 2024 03:05
@qiaozha qiaozha requested a review from wanlwanl August 26, 2024 03:23
@qiaozha
Copy link
Member Author

qiaozha commented Aug 26, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -1035,6 +1035,198 @@ describe("modular special union serialization", () => {
true
);
});

it("should generate serialize util if there're special discriminated union within a anonymous model", async () => {
Copy link
Member

@MaryGao MaryGao Aug 28, 2024

Choose a reason for hiding this comment

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

Could you simplify this case with the root issue maybe removing un-necessary doc and decorators? Does the anomymous model happen in body type?

Copy link
Member

@MaryGao MaryGao Aug 29, 2024

Choose a reason for hiding this comment

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

I don't like the idea of directly copying the spec into our UTs because this would introduce a lot of un-necessary changes and also not good for review. we may have plan to remove smoke tests and add more IT or UTs. if these test cases are not small, we may not be effective either.

Copy link
Member

Choose a reason for hiding this comment

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

I found all other tests in this file test in the same way, since it's quite urget to meet the MPG automation deadline, we may need to keep the same method to test. @raych1 , what do you think?
Would you drive the test improvement in the future? @MaryGao

@qiaozha qiaozha merged commit 9a9fcca into Azure:main Aug 29, 2024
15 checks passed
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.

enum member name openai assistant modular generation failure
3 participants