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

update error message to align with playwright-go update #1299

Conversation

charlottecampbell193
Copy link

@charlottecampbell193 charlottecampbell193 commented Jun 24, 2024

New version of playwright-go 'run.go' file added a check on if the driver is up to date. Updated TestNoBrowserDriverFail in browser_test.go to look for the new error message.

	driver, err := NewDriver(transformRunOptions(options))
	if err != nil {
		return nil, fmt.Errorf("could not get driver instance: %w", err)
	}
	**up2date, err := driver.isUpToDateDriver()
	if err != nil || !up2date {
		return nil, fmt.Errorf("please install the driver (v%s) and browsers first: %w", playwrightCliVersion, err)
	}**

dependabot bot and others added 3 commits June 11, 2024 00:15
Bumps [github.com/playwright-community/playwright-go](https://github.com/playwright-community/playwright-go) from 0.4201.1 to 0.4401.0.
- [Release notes](https://github.com/playwright-community/playwright-go/releases)
- [Commits](playwright-community/playwright-go@v0.4201.1...v0.4401.0)

---
updated-dependencies:
- dependency-name: github.com/playwright-community/playwright-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@charlottecampbell193 charlottecampbell193 changed the base branch from dependabot/go_modules/github.com/playwright-community/playwright-go-0.4401.0 to master June 24, 2024 08:56
Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

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

Minor clarification comment

@@ -102,7 +102,7 @@ func TestNoBrowserDriverFail(t *testing.T) {
client, _ := New(account)
_, err := client.Authenticate(loginDetails)
assert.Error(t, err)
assert.ErrorContains(t, err, "could not start driver")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for this change @charlottecampbell193 - is there a scenario where the error "could not start driver" will be thrown? Or has that been completely removed?

Choose a reason for hiding this comment

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

@mapkon that error can still be thrown but not with the current test. A new test would need to be created that has the correct driver installed but has some other issue that is preventing it from starting. I'm not sure what scenario that would be though.

Copy link
Contributor

@mapkon mapkon Jun 26, 2024

Choose a reason for hiding this comment

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

You are right since the current error message is probably faulty, and playwright has fixed their API. The current test was exercising the scenario where there is completely no driver, so it makes sense to throw the please install the driver message. The comment on the test gave it away:
// Test that if download directory does not have browsers, it fails with expected error message

I imagine that we should have a test that launches a directory with an existing driver that we know won't "start", and see if that will fire it up (probably an invalid browser binary or similar). I wouldn't spend more than a day on it if you cannot get it to start.

Lastly, remember to update the README on line 575 to reflect the new message that will get thrown if the user attempts use without installing the drivers (like on first time use).

Choose a reason for hiding this comment

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

I was not able to recreate that error message. When I had an invalid browser binary the error I got was 'playwright: spawn UNKNOWN'. Tried a bunch of stuff messing around with it and got many other unhelpful messages but not could not start driver.

README has been updated - let me know your thoughts on the update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suffices for me. We can merge it now or wait for #1302 to be fixed (so we can run all the tests). Let me know

Charlotte Campbell added 2 commits June 27, 2024 12:03
…laywright-go-0.4401.0' of github.com:charlottecampbell193/saml2aws into dependabot/go_modules/github.com/playwright-community/playwright-go-0.4401.0
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.19%. Comparing base (99d6fe4) to head (ed387cc).
Report is 14 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1299   +/-   ##
=======================================
  Coverage   42.19%   42.19%           
=======================================
  Files          54       54           
  Lines        6456     6456           
=======================================
  Hits         2724     2724           
  Misses       3283     3283           
  Partials      449      449           
Flag Coverage Δ
unittests 42.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mapkon mapkon merged commit 2dffe79 into Versent:master Jun 27, 2024
8 checks passed
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.

3 participants