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

Avoid calling googlechromelabs.github.io when offline + ffx #12709

Closed
wants to merge 3 commits into from

Conversation

step21
Copy link

@step21 step21 commented Apr 16, 2024

Proposed changes

//: # Ideally this allows tests with Firefox to work offline and not call googlechromelabs.github.io (and thus fail). As discussed in #12705

Types of changes

  • Polish (an improvement for an existing feature)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (improvements to the project's docs)
  • Specification changes (updates to WebDriver command specifications)
  • Internal updates (everything related to internal scripts, governance documentation and CI files)

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Backport Request

//: # (The current main branch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against the v8 branch.)

  • This change is solely for v9 and doesn't need to be back-ported
  • Back-ported PR at #12718

Further comments

//: # Right now at least locally there was one test failure, but I am not sure how bad this is or how to solve it. So I wanted to get started on the PR anyway for now. It seems the test expect a Windows version, maybe it failed for me on macOS because of that?

FAIL  packages/wdio-utils/tests/node/utils.test.ts > driver utils > setupPuppeteerBrowser > should install chrome stable if browser is not found
AssertionError: expected "spy" to be called 1 times, but got 0 times
 ❯ packages/wdio-utils/tests/node/utils.test.ts:147:36
    145|                 executablePath: '/path/to/stable'
    146|             })
    147|             expect(resolveBuildId).toBeCalledTimes(1)
       |                                    ^
    148|             expect(resolveBuildId).toBeCalledWith('chrome', 'windows', '116.0.5845.110')
    149|         })

Reviewers: @webdriverio/project-committers

Copy link

linux-foundation-easycla bot commented Apr 16, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: step21 / name: Florian Idelberger (2c1a1af, c59d086)
  • ✅ login: christian-bromann / name: Christian Bromann (6a1b898)

@christian-bromann
Copy link
Member

christian-bromann commented Apr 18, 2024

Can we raise the same PR to the v8 branch please? Let's also get the unit test fixed:

diff --git a/packages/wdio-utils/tests/node/utils.test.ts b/packages/wdio-utils/tests/node/utils.test.ts
index 2e7e8e0c9..7020a8f1b 100644
--- a/packages/wdio-utils/tests/node/utils.test.ts
+++ b/packages/wdio-utils/tests/node/utils.test.ts
@@ -140,11 +140,11 @@ describe('driver utils', () => {
         it('should install chrome stable if browser is not found', async () => {
             vi.mocked(detectBrowserPlatform).mockReturnValueOnce('windows' as any)
             vi.mocked(locateChrome).mockResolvedValue('/path/to/stable')
-            await expect(setupPuppeteerBrowser('/foo/bar', {})).resolves.toEqual( {
+            await expect(setupPuppeteerBrowser('/foo/bar', {})).resolves.toEqual({
                 browserVersion: '116.0.5845.110',
                 executablePath: '/path/to/stable'
             })
-            expect(resolveBuildId).toBeCalledTimes(1)
+            expect(resolveBuildId).toBeCalledTimes(0)
             expect(resolveBuildId).toBeCalledWith('chrome', 'windows', '116.0.5845.110')
         })

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Thank you!

👍

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Apr 18, 2024
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

@step21 can you check the failing test?

@step21
Copy link
Author

step21 commented Apr 22, 2024

@step21 can you check the failing test?

For the failing test, most obvious to me seems that when expect(resolveBuildId).toBeCalledTimes(0), I also need to delete the second line expect(resolveBuildId).toBeCalledWith('chrome', 'windows', '116.0.5845.110') as ofc if it is not called, it is not called with something. This might however negate the point of the test. I do not have that much knowledge about the webdriverio codebase, but maybe it would need separate tests for when chrome is previously installed, and when it is not?

@christian-bromann
Copy link
Member

@step21 it is likely that we don't clear the mock beforehand.

@christian-bromann
Copy link
Member

Closing as this hand landed as part of #12720

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants