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

add test for error deserialization in op with param name 'models' #234

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

iscai-msft
Copy link
Contributor

@iscai-msft iscai-msft commented Nov 9, 2020

Python ran into an issue for (error) model deserialization when our operation had an input parameter named models.
(python ex). Adding test to make sure other languages don't run into this conflict. Making this optional coverage, since right now it seems p specific to how python deserializes models. Other languages shouldn't have a problem passing the test though

models_param = req.query['models']
if (models_param == 'value1') {
res.status(500).json(sadCasper);
coverage['sendErrorWithParamNameModels']++;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make it optional, since this might be python specific

…into add_operation_with_error_model_and_param_model

* 'master' of https://github.com/Azure/autorest.testserver:
  add test for first response no items, second response with items (#235)
@iscai-msft iscai-msft merged commit 5b70058 into master Nov 10, 2020
@iscai-msft iscai-msft deleted the add_operation_with_error_model_and_param_model branch November 10, 2020 22:29
iscai-msft added a commit that referenced this pull request Nov 11, 2020
…into special_paging

* 'master' of https://github.com/Azure/autorest.testserver:
  add test for error deserialization in op with param name 'models' (#234)
  add test for first response no items, second response with items (#235)
@pakrym
Copy link
Contributor

pakrym commented Nov 11, 2020

I think if we are adding tests to the test server they should be useful for every language. This seems to be more of a python-specific test that other languages would have to implement without much benefit.

I see two things we can do here:

  1. Take keywords and special names from all the languages we support and throw it into the test. (see https://github.com/Azure/autorest.csharp/blob/feature/v3/test/TestProjects/NameConflicts/NameConflicts.json#L190)
  2. Have tests like this be part of autorest.python.

@iscai-msft
Copy link
Contributor Author

I see @pakrym, sorry I merged this before you could comment.

The more I read your message the more it makes sense. Opened an issue to transfer this to the autorest python repo (so far we don't have any swaggers specific to that repo so need to set that up). In the meantime, it's tagged as optional coverage, so it shouldn't break any languages if they choose not to implement it

iscai-msft added a commit that referenced this pull request Nov 24, 2020
…into multiapi_diff_signatures

* 'master' of https://github.com/Azure/autorest.testserver:
  Pass through program exit code when using start-autorest-express (#240)
  fix value retval (#237)
  add test for error deserialization in op with param name 'models' (#234)
  add test for first response no items, second response with items (#235)
  bump version after failing to in no op swagger addition (#231)
  add swagger for no operations (#230)
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