Instance discovery remains cloud local on known clouds#875
Instance discovery remains cloud local on known clouds#875
Conversation
githubursul
left a comment
githubursul
left a comment
There was a problem hiding this comment.
Thank you for the changes. I made some recommendations.
| continue | ||
| self._test_given_host_and_tenant(host, "common") | ||
|
|
||
| def test_new_sovereign_hosts_should_be_known_authorities(self): |
There was a problem hiding this comment.
This test is not durable. The set of "new" authorities will change with time. And I also don't see how to make it truly useful, since it tests that the values have been added to the map.
|
|
||
| @patch("msal.authority._instance_discovery") | ||
| @patch("msal.authority.tenant_discovery") | ||
| def test_new_sovereign_hosts_should_build_authority_endpoints( |
There was a problem hiding this comment.
This test is not durable, since the set of new authorities will change with time. But it is useful - it verifies tha tthe string added to wel-known authority makes up an authority.
I suggest that all well-known authorities from the set are tested to make it durable.
|
|
||
| @patch("msal.authority._instance_discovery") | ||
| @patch("msal.authority.tenant_discovery") | ||
| def test_known_authority_should_use_same_host_and_skip_instance_discovery( |
There was a problem hiding this comment.
I think this test should do it for every well known authority, not just for a single one. Essentially, it should validate the semantic of "well-knowness"
githubursul
left a comment
githubursul
left a comment
There was a problem hiding this comment.
Thank you for the changes. I made some recommendations. Please address before merging.
2 participants
No description provided.