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

SetupEdge - user-data-dir - admin support #452

Closed
wants to merge 1 commit into from

Conversation

mlipok
Copy link
Contributor

@mlipok mlipok commented Apr 13, 2023

Pull request

Proposed changes

Add support for runing MS EDGE with elevated to admin

#RequireAdmin

Checklist

  • I have read and noticed the CODE OF CONDUCT document
  • I have read and noticed the CONTRIBUTING document
  • I have added necessary documentation or screenshots (if appropriate)

Types of changes

  • Bugfix (change which fixes an issue)
  • Feature (change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (functional, structural)
  • Documentation content changes
  • Other (please describe)

What is the current behavior?

using SetupEdge with elevation to Admin will hit error

What is the new behavior?

works properly

Additional context

https://www.autoitscript.com/forum/topic/210071-webdriver-and-admin-rights

https://www.autoitscript.com/forum/topic/210071-webdriver-and-admin-rights/?do=findComment&comment=1516684

System under test

MS EDGE

@mlipok
Copy link
Contributor Author

mlipok commented Apr 13, 2023

Should we also add relevant changes for other browsers ?
I mean for any case.

Copy link
Owner

@Danp2 Danp2 left a comment

Choose a reason for hiding this comment

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

IMO, this shouldn't be added as "default" functionality just because MSEdge is misbehaving when run under Admin privileges. I would recommend making the following changes --

  • Comment out this line by default
  • Add a comment block explaining that the line should be uncommented if issues are encountered when running the demo script under Admin privileges.

@mlipok
Copy link
Contributor Author

mlipok commented Apr 13, 2023

Maybe check if is admin .. ?

@Danp2
Copy link
Owner

Danp2 commented Apr 13, 2023

Maybe check if is admin .. ?

That's an option. However, I suggest that we take a step back and analyze the purpose of wd_demo. IMO, it is designed to demonstrate some of the basic functionality of the WD UDF. It isn't designed to handle every possible scenario or strange configuration that someone can throw at it.

There are obvious times where exceptions are made. One example is where the 'binary' option was added to the Capabilities string to avoid issues with 32-bit versions of some webdrivers. This is a common error that could occur, so it made sense to include the fix.

I'm not so sure that this current issue would fall into that same category of commonly occurring. Until today, I've never run wd_demo with #requireadmin, and I highly doubt that others need this functionality in this demo script.

@Danp2
Copy link
Owner

Danp2 commented Apr 13, 2023

MicrosoftEdge/EdgeWebDriver#80

@Danp2
Copy link
Owner

Danp2 commented Apr 14, 2023

So this behavior is by design per Microsoft. Also, further testing has shown that this change doesn't actually fix the issue,

@Danp2 Danp2 closed this Apr 14, 2023
@mlipok mlipok deleted the ml__SetupEdge_admin branch April 20, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants