-
Notifications
You must be signed in to change notification settings - Fork 208
Arlington automation #165
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
Arlington automation #165
Conversation
|
cc: @henrik-me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really THANK YOU for your effort here! Seeing all those actual test cases passing in CI, gives us a high confidence on the recent Arlington testing work. For that reason, I think you can go ahead to document our findings in the test, into that Arlington workitem, and close that part of work there.
For this PR here, we do have room for improvement, from a code maintenance perspective. I leave some comments below (and more to come). Let's polish it a little bit more in next couple days. Again, this part is NOT a blocker for the Arlington test effort.
tests/test_e2e.py
Outdated
| self._test_username_password( | ||
| password=self.get_lab_user_secret(config["lab_name"]), **config) | ||
|
|
||
| def test_arlington_acquire_token_by_client_secret(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historically, there was no explicit test case to cover confidential client in world-wide (WW) cloud. (We implicitly tested it by creating a confidential client as test infrastructure, though.)
That's why I LOVE seeing you add an explicit test case for confidential client this time! Let's see if we can follow the suggestion above, to try to backport this test case for WW cloud, in a reusable style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats good to have. I think that should go in a separate PR though once this is merged. Lets keep the scope of this PR to Arlington test cases ? Let me know if that works for you. If yes, I can go and create an issue tracking this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not doing this now, then please create a github issue to refactor test cases for multiple cloud enablement. You can point to this comment. Expect that I will be asking for the same automation to run in all the clouds we support and that code should be well structured to handle that (similar discussion happening in other repo's at this time)
|
I would have expected to see something like this: test_allenvironments(config) test_environment(environment, config) test_devicecode(environment, ....) {} Are there many differences between the environments making that not easy to do? Am I reading the code in the wrong way? |
|
There seems to be several references to client id's etc. in the tests. Any reason the lab API is not used to get those as well as the secrets? |
|
Hey @henrik-me,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A batch of editorial changes, many of them are provided with the Github Suggestion feature, for potentially easier adoption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the 2 last comments to this PR. We will be merging this soon. :-)
tests/test_e2e.py
Outdated
| username="b2clocal@msidlabb2c.onmicrosoft.com", | ||
| password=self.get_lab_user_secret("msidlabb2c"), | ||
| scope=["https://msidlabb2c.onmicrosoft.com/msidlabb2capi/read"], | ||
| scope=["https://msidlabb2c.onmicrosoft.com/msidlabb2capi/read"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| scope=["https://msidlabb2c.onmicrosoft.com/msidlabb2capi/read"] | |
| scope=["https://msidlabb2c.onmicrosoft.com/msidlabb2capi/read"], |
Reasoning: PEP 8 on Trailing commas
PS: It is not necessarily that I paid attention to this kind of details. It is the github PR review UI highlighting the different lines in red-and-green in order to grab reviewer's attention. So, a universal hint for a PR author is to conduct a self-review on github PR UI to see whether each of those red-and-green snippets is intended and/or necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the effort! The entire end-to-end test suite looks more organized now. 👍
Let's squash and merge it! 🚢
E2E Test Scenarios added:
Besides, acquire token silent is tested in each of the flows individually (except acquire token by client secret)