-
Notifications
You must be signed in to change notification settings - Fork 10
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
SC-tests/Graphical: add timeout and HTML file changes #39
Conversation
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.
more question to consider. Otherwise looks good.
@@ -59,7 +59,11 @@ def test_login_with_sc(local_user, required): | |||
r'.*user=' + local_user.username + r'@shadowutils.*' | |||
) | |||
|
|||
with Authselect(required=required), local_user.card(insert=True), GUI() as gui: | |||
# Directory name for the gui to create the html file | |||
res_dir = str(int(time())) + "_login_with_sc" |
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.
I thought if the test name could be derived from the function name automatically, but it turns out there is no __func__
as in C. On the other hand, there is a way to inspect the call stack, which could make generating this path inside of the GUI constructor possible without the need to change the tests using inspect
:
https://stackoverflow.com/questions/5067604/determine-function-name-from-within-that-function
Would it be something usable here?
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.
@Jakuje I was thinking of it as well and I saw this stack but since it is added manually in every test case I do not know if it is really worth doing some more operations rather than just putting it as a string. I don't really see a benefit, if you do please let me know and I can implement it.
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.
Given that there are just few tests, I think that is ok. It was just a suggestion. I think we are good to go.
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.
Oh, now I get what you mean. I can search if that is possible to be implemented! We can just do the function that is called the GUI() if nothing was passed. Clever!
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.
@Jakuje implemented in both PRs
0e66e2e
to
6a32ccf
Compare
with Authselect(required=required), local_user.card(insert=True), GUI() as gui: | ||
with (GUI() as gui, | ||
Authselect(required=required), local_user.card(insert=True)): |
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.
I think the order here matters because we will get the authselect logs in gui HTML log, is it correct? This should be mentioned in the commit message, as it still talks about the res_dir, which was moved to the sc autolib.
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.
@Jakuje Done
1) added timeout to all assert_text calls. 2) Changed order between GUI and Authselect class calling in context manager so the HTML file will include Authselect logs. This happens because HTML file initialization happens in GUI __init__ func. 3) fixed minor issues in test_insert_card_prompt.
manager so the HTML file will include Authselect logs. This happens
because HTML file initialization happens in GUI init func.
connected to SCAutolib PR#135