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

Removing type validation for errors #19361

Merged
merged 4 commits into from
Dec 17, 2021

Conversation

JonathanCrd
Copy link
Member

Fixes: #19355

Removing the type validation for the errors since they are not needed with the current typescript version and they were introduced in PR: #19245

@JonathanCrd JonathanCrd self-assigned this Dec 14, 2021
@ghost ghost added the App Configuration Azure.ApplicationModel.Configuration label Dec 14, 2021
@HarshaNalluru
Copy link
Member

/azp run js - app-configuration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HarshaNalluru
Copy link
Member

image
sample job failure is expected because we don't create keyvault resources.

@@ -163,9 +162,6 @@ export async function assertThrowsRestError(
await testFunction();
assert.fail(`${message}: No error thrown`);
} catch (err) {
if (!(err instanceof RestError)) {
Copy link
Member

Choose a reason for hiding this comment

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

Did the pipeline fail because of this line should assert Error instead? The other two look fine to me if we expect them to be Error. Since the code is already merged in main, we could leave them there if they are correct, which would save us from doing it again later when moving to TypeScript v4.4

Copy link
Member

Choose a reason for hiding this comment

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

min-max pipelines were failing, which is why this PR was needed in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect that we were getting some unexpected errors in the min-max testing. It should have passed this check if it's expected. Even if we removed this check, it would fail below. I was thinking that we might want to do instance of Error check here, and let it fail later.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main problem with the pipelines was related to this check, err was not an instance of RestError.
I can try checking for instance of Error and see if we don't get the "Error is not recognized" on the pipeline

Copy link
Member

Choose a reason for hiding this comment

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

I suspect that it is caused by two different versions of core-http in min/max testing. Pushed an update. This hopefully should fix the build.

@HarshaNalluru
Copy link
Member

I don't really care about how we are resolving the issue as long as the builds(and live tests) are green.

That said, @JonathanCrd can you sync up with @jeremymeng and @KarishmaGhiya to come up with a plan not just for this PR, but also for other SDKs that are/(might be) affected by the same problem?

In min/max testing `instanceof RestError` failed. I suspect that the RestError
type and the instance are from two different versions of core-http.
@jeremymeng
Copy link
Member

/azp run js - app-configuration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng
Copy link
Member

The sample has been failing for a while and unrelated. Merging.

@jeremymeng
Copy link
Member

/check-enforcer override

@jeremymeng jeremymeng merged commit 4550b69 into Azure:main Dec 17, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Jun 15, 2022
Feature/cplat 2022 04 04 (Azure#19467)

* set up feature branch

* Update cloud service swagger file and examples (Azure#19311)

* Update cloud service swagger file and examples

* Update cloudService.json

Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>

* move systemData object to common.json

* Add CloudServiceSlotType to swagger api 2022-04-04 (Azure#19361)

Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>

* update with prettier

* resolve readme input

* add missing example

* fix example error, update common with latest change.

* set up branch

* change folder name

* change folder name

Co-authored-by: Arpit Khandelwal <arpitkhandelwal@hotmail.com>
Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>
Co-authored-by: Theodore Chang <theodore.l.chang@gmail.com>
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Jun 23, 2022
CloudService 2022-04-04 release  (Azure#19468)

* set up branch

* Feature/cplat 2022 04 04 (Azure#19467)

* set up feature branch

* Update cloud service swagger file and examples (Azure#19311)

* Update cloud service swagger file and examples

* Update cloudService.json

Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>

* move systemData object to common.json

* Add CloudServiceSlotType to swagger api 2022-04-04 (Azure#19361)

Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>

* update with prettier

* resolve readme input

* add missing example

* fix example error, update common with latest change.

* set up branch

* change folder name

* change folder name

Co-authored-by: Arpit Khandelwal <arpitkhandelwal@hotmail.com>
Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>
Co-authored-by: Theodore Chang <theodore.l.chang@gmail.com>

* prettier fix

Co-authored-by: Theodore Chang <theodore.l.chang@gmail.com>
Co-authored-by: Arpit Khandelwal <arpitkhandelwal@hotmail.com>
Co-authored-by: Arpit Khandelwal <arkhande@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[App Config] Min-Max tests are failing related to assert changes
3 participants