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

Remove empty "test" #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove empty "test" #15

wants to merge 1 commit into from

Conversation

gaelicWizard
Copy link
Contributor

I'm generally opposed to silencing a warning by adding a dummy block that expressly does not do what the warning recommends. If there's no way to test this, then the warning is correct: there's no test. If there is a way to test this, then adding "true" is not a good test.

Some possible options to write a good test:

  • verify that osascript runs?
  • set some kind of timeout and then run ssh-askpass?
  • syntax validation of script?
  • syntax validation of plist?

Here's an idea: asynchronously run ssh-askpass with known inputs, then run a second osascript which passes "enter" or clicks "OK" to the waiting window, then validate the return value.

I'm generally opposed to silencing a warning by adding a dummy block that expressly does *not* do what the warning recommends. If there's no way to test this, then the warning is correct: there's no test. If there is a way to test this, then adding "true" is not a good test.

# Conflicts:
#	Formula/ssh-askpass.rb
@theseal
Copy link
Owner

theseal commented Jul 18, 2021

I tried to do what you suggests by executing two parallel scripts but couldn't get it to work. Not sure which window/dialog to aim for and it seems like it requires Accessibility permissions which I think gets a lot more overhead then we want.

Since this PR no longer can get merged with out conflict I suggest that we leave the code as is until someone can create a real test.

@gaelicWizard
Copy link
Contributor Author

I'll try to write something up. I think it'll boil down to: osascript -e 'tell application "System Events" to tell process "System Events" enter' or something like that

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.

2 participants