-
Notifications
You must be signed in to change notification settings - Fork 567
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
Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 #1006
Changes from all commits
ae4b316
f23b702
ec1b693
1050683
8e50c76
2753342
d2e851f
006178e
b1946bf
6956721
1dc5025
c539464
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,13 +39,29 @@ func TestValidate(t *testing.T) { | |
client, err := New(account) | ||
assert.Nil(t, err) | ||
loginDetails := &creds.LoginDetails{ | ||
URL: "https://google.com/", | ||
URL: "https://google.com/", | ||
DownloadBrowser: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to test fail without download? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a word, yes. It should be easy to copy/paste this test without the flag set and check if the error is not nil since I believe that's how the error is being propagated up to the user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, so I've added the test, though the change was larger than I had thought it'll be. I have added a new config parameter to allow one to set the directory that Playwright installs into though I've purposely not documented it. I did this way because otherwise my test was going to be sensitive if a previous test ran and installed playwright before this test which introduces all sorts of headaches. Also couldn't think of a clean way to tear that down without being a royal pain to do so. So instead, you technically can now choose where you download the drivers to, defaulting to the default location if you don't care. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left the existing test so that it downloads the drivers to the default location though an argument could be made it shouldn't. |
||
} | ||
resp, err := client.Authenticate(loginDetails) | ||
assert.Nil(t, err) | ||
assert.Equal(t, resp, response) | ||
} | ||
|
||
// Test that if download directory does not have browsers, it fails with expected error message | ||
func TestNoBrowserDriverFail(t *testing.T) { | ||
account := &cfg.IDPAccount{ | ||
Headless: true, | ||
BrowserDriverDir: t.TempDir(), // set up a directory we know won't have drivers | ||
} | ||
loginDetails := &creds.LoginDetails{ | ||
URL: "https://google.com/", | ||
} | ||
client, _ := New(account) | ||
_, err := client.Authenticate(loginDetails) | ||
assert.Error(t, err) | ||
assert.ErrorContains(t, err, "could not start driver") | ||
} | ||
|
||
func fakeSAMLResponse(page playwright.Page, loginDetails *creds.LoginDetails) (string, error) { | ||
return response, nil | ||
} | ||
|
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.
will this (re)download/install at every run?
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.
It shouldn't re-download since playwright-go does have that caching mechanism. Just by manually invoking the relevant
playwright install
command it doesn't re-download the binary, at least not without first nuking the relevant cache.With this code, if you were to set it in the config file, it would functionally bring you back to the 2.36.4 behavior where it always checks if you have the drivers or not. If you do, it outputs the relevant message about how the drivers are installed but with no progress bar for downloading the binaries.
Alternatively, if you don't set it in the config, it will only download the driver if you set the relevant flag.