-
Notifications
You must be signed in to change notification settings - Fork 29
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
Performance Testing #58
Conversation
Release 0.1.1
MSAL Python Extensions 0.1.2
MSAL Extensions for Python 0.1.3
MSAL Extensions for Python 0.1.3 (part 2 of 2)
Release 0.2.0
Release 0.2.1
Release 0.2.2
tests/test_cache_lock_file_perf.py
Outdated
|
||
for i in processes: | ||
i.join() | ||
validate_thread_locking_file(num_of_processes) |
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.
Please refactor the test driver to be a stand alone CLI script (I also copy @bgavrilMS 's expectation on that CLI script in this PR's description), and then have the test validation as a unit test case.
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 @abhidnya13 's effort so far! A good starting point.
Eventually this PR will be merged as part of our code base. So, now let's work on polish this proof-of-concept.
Yes, this definitely is a very rough draft and I would be making a lot more changes, stay tuned ! |
Thanks for reviewing @bgavrilMS , I am supposed to make changes, created a draft PR as Ray mentioned it would be good to have a draft PR out for now and then if possible he can test it on his Linux machine as well. Will push updates soon on a more polished implementation. However I did test it on my Mac and I noticed that processes ~50 is when it starts to give cache errors. I also noticed on Mac that it fails after ~400 threads but as you pointed out MSAL ensures thread safety would possibly mean we would not get these errors for cross thread testing. |
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.
In terms of performance/stress testing, this PR did its job! Well done! I'm leaving an approval here.
With regarding to @bgavrilMS 's expectation on "produce a simple console app that ..." (which I pasted inside this PR's description), Bogdan how/where do you want that console app to be available? @abhidnya13 can wrap this existing function as a stand-alone script which is still hosted inside this repo, and then your MSAL EX .Net cross-platform test can access it. Will that work?
The goal is to check that our extensions work side by side. First manually, ideally automated. I think having a console app / script will help this. I won't be able to look at this work item for a while though, we should discuss if someone else can pick it up. |
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.
Nice finishing touch! Love your desire to perfection! :-) Approving, again.
Reference:
@bgavrilMS mentioned: