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

[pull] main from microsoft:main #69

Merged
merged 59 commits into from
Oct 24, 2023

Conversation

pull[bot]
Copy link

@pull pull bot commented Oct 14, 2023

See Commits and Changes for more details.


Created by pull[bot]

Can you help keep this open source service alive? 💖 Please sponsor : )

yury-s and others added 5 commits October 13, 2023 14:02
#27603)

…owser fails immediately"

The test has been failing on Windows since
f212fd1
Speculative fix. The test marked as failed => we reuse the browser
without closing the context.
- Do not write from workers.
- Remove marker files.
- Always try/catch reading from fs.

This mostly reverts #26830
and #26353.

Fixes #27592.
@restack-app
Copy link

restack-app bot commented Oct 14, 2023

No applications have been configured for previews targeting branch: main. To do so go to restack console and configure your applications for previews.

@commit-lint
Copy link

commit-lint bot commented Oct 14, 2023

Tests

Features

Chore

Bug Fixes

Documentation

Contributors

yury-s, playwrightmachine, dgozman, mxschmitt, pavelfeldman, jleedev, sand4rt, boyum, olexandr13

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@pr-code-reviewer
Copy link

pr-code-reviewer bot commented Oct 14, 2023

👋 Hi there!

  1. Update the Chromium, Firefox, and WebKit version badges to their latest versions to provide accurate information.
  2. Check if the hyperlink URLs are still valid and update them if necessary.
  3. Review the rest of the code-diff for any other issues that may need improvement or updating.


Automatically generated with the help of gpt-3.5-turbo.
Feedback? Please don't hesitate to drop me an email at webber@takken.io.

@instapr
Copy link

instapr bot commented Oct 14, 2023

The pull request description is unclear and lacks specific details about the changes made. Please provide a more informative and descriptive summary of the changes.

@sweep-ai
Copy link

sweep-ai bot commented Oct 14, 2023

Apply Sweep Rules to your PR?

  • Apply: Leftover TODOs in the code should be handled.
  • Apply: All new business logic should have corresponding unit tests in the tests/ directory.
  • Apply: Any clearly inefficient or repeated code should be optimized or refactored.

@pr-explainer-bot
Copy link

Pull Request Review

Hey there! 👋 Here's a summary of the previous results for the pull request review:

Changes

  1. Updated revision value in browsers.json file from 1925 to 1926.
  2. Added createHttpServer function to the import statement in playwrightServer.ts file.
  3. Added this._server property to the PlaywrightServer class in playwrightServer.ts file.
  4. Added an upgrade event listener to the server in playwrightServer.ts file.
  5. Added a check for pathname and user-agent in the upgrade event listener in playwrightServer.ts file.
  6. Added a handleUpgrade call to the upgrade event listener in playwrightServer.ts file.
  7. Added a userAgentVersionMatchesErrorMessage function to the import statement in userAgent.ts file.
  8. Added a userAgentVersionMatchesErrorMessage function to check the user agent version in userAgent.ts file.
  9. Added an error message for version mismatch in userAgentVersionMatchesErrorMessage function in userAgent.ts file.
  10. Updated the getPlaywrightVersion function to use the PW_VERSION_OVERRIDE environment variable if available in userAgent.ts file.
  11. Added a check for version mismatch in the start function of RunServer class in remoteServer.ts file.
  12. Added a test case for version mismatch in browsertype-connect.spec.ts file.
  13. Added a test case for version mismatch in browsertype-launch.spec.ts file.

Suggestions

  • In playwrightServer.ts file, consider using a more descriptive name for the upgrade event listener function.
  • In playwrightServer.ts file, consider extracting the logic for handling the upgrade request into a separate function for better readability.

Bugs

  • In playwrightServer.ts file, there is a potential bug in the upgrade event listener where the pathname check may not work as expected. It should use the url.pathname instead of new URL('http://localhost' + request.url!).pathname.
  • In userAgent.ts file, there is a potential bug in the userAgentVersionMatchesErrorMessage function where the error message is not returned if the user agent cannot be parsed. Consider adding a default error message in this case.
  • In browsertype-connect.spec.ts file, there is a potential bug in the test case...

Improvements

  • One place in the code that could be refactored for better readability is the getFromCompilationCache function at line 73. Here's an improved version of the function:
export function getFromCompilationCache(filename: string, hash: string, moduleUrl?: string): { cachedCode?: string, addToCache?: (code: string, map?: any) => void } {
  const cache = memoryCache.get(filename);
  if (cache?.codePath) {
    try {
      return { cachedCode: fs.readFileSync(cache.codePath, 'utf-8') };
    } catch {
      // Not able to read the file - fall through.
    }
  }
  const cachePath = calculateCachePath(filename, hash);
  const codePath = cachePath + '.js';
  const sourceMapPath = cachePath + '.map';
  try {
    const cachedCode = fs.readFileSync(codePath, 'utf8');
    _innerAddToCompilationCache(filename, { codePath, sourceMapPath, moduleUrl });
    return { cachedCode };
  } catch {
  }
  return {
    addToCache: (code: string, map: any) => {
      if (isWorkerProcess()) {
        return;
      }
      fs.mkdirSync(path.dirname(cachePath), { recursive: true });
      if (map) {
        fs.writeFileSync(sourceMapPath, JSON.stringify(map), 'utf8');
      }
      fs.writeFileSync(codePath, code, 'utf8');
      _innerAddToCompilationCache(filename, { codePath, sourceMapPath, moduleUrl });
    }
  };
}

Rating

Rating: 7/10
Criteria: readability, performance, security

That's it for the summary! Let me know if you need any further assistance. 😄

playwrightmachine and others added 19 commits October 14, 2023 16:17
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
`node utils/render_release_notes.mjs js 1.39`
Also, fix some missing paths in download lists.
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This fixes:

```
Run npm audit --omit dev
# npm audit report

@babel/traverse  <7.23.2
Severity: critical
Babel vulnerable to arbitrary code execution when compiling specifically crafted malicious code - https://github.com/advisories/GHSA-[6](https://github.com/microsoft/playwright/actions/runs/6535308689/job/17744452034?pr=27631#step:10:7)[7](https://github.com/microsoft/playwright/actions/runs/6535308689/job/17744452034?pr=27631#step:10:8)hx-6x53-jw[9](https://github.com/microsoft/playwright/actions/runs/6535308689/job/17744452034?pr=27631#step:10:10)2
fix available via `npm audit fix`
node_modules/@babel/traverse

1 critical severity vulnerability

To address all issues, run:
  npm audit fix
Error: Process completed with exit code 1.
```
On Linux platforms, specifically check that process.arch is x64, rather
than treating it as 'not arm64'.

Treat Raspbian's /etc/os-release file as Debian.

Document the supported platforms somewhat.

Fixes #27453
playwrightmachine and others added 14 commits October 19, 2023 16:15
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This is not referenced anywhere and not getting updated.
When merging trace files, we sometimes left open read streams from the
zip, which prevents it from being removed.

Fixes #27286.
Fixes #27691

Signed-off-by: Max Schmitt <max@schmitt.mx>
…bindings (#27706)

**Description**

When a language port was using Inspector with the "Locator Picker"
feature, it only recognised JavaScript as a language by default. As a
workaround the user was able to click record, interact with the page and
then the language would be correctly used -> csharp e.g. would work in
the "Locator Picker".

**Why?**

Our language bindings are setting `PW_LANG_NAME=<sdkLanguage>` env var
-> good. Our recorder harness also uses this along its internal state
here:


https://github.com/microsoft/playwright/blob/b9b289b6415515b1b8e1a2524ed6425c8992af5a/packages/playwright-core/src/server/recorder.ts#L369

and it gets used here (no parameter means: we use the first language
aka. primary language):


https://github.com/microsoft/playwright/blob/b9b289b6415515b1b8e1a2524ed6425c8992af5a/packages/playwright-core/src/server/recorder.ts#L95

The only issue is that the Inspector frontend in the beginning does not
know which language it should use and pass over to the server side, it
then falls back to JavaScript.

**Proposed fix**

Instead of passing it over from the frontend to the server side, we just
always use it from the server side, aka. "currentLanguage". When the
user switches languages in the frontend, "currentLanguage" already gets
updated properly via the "fileChanged" event.

microsoft/playwright-dotnet#2718

---------

Signed-off-by: Max Schmitt <max@schmitt.mx>
@coderabbitai
Copy link

coderabbitai bot commented Oct 22, 2023

Important

Review Skipped

Reviews are disabled for bot users.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

Th3S4mur41 and others added 14 commits October 23, 2023 12:59
)

Before this fix, unhandled error during test.fail():
- marks this test as "interrupted";
- fails next test in the file with "fatal error".

After this fix:
- marks this test as "failed as expected";
- restarts worker for the next test.
- remove `onlyStartedTests` in favor of explicit branch with comments;
- produce one "test not found" error per test instead of a single large
error;
- extract `_failTestWithErrors` from `_massSkipTestsFromRemaining`.
We saw in the past a lot of users reporting issues like this. Maybe a
more readable error helps.

Before they just saw:

```
Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'register')
  at index.38834ec3.js:1:3775
  at index.38834ec3.js:1:4006
```

Fixes #27655
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
…27742)

Motivation: As of today when a user inspects a Locator which is a xpath,
it won't work if the user has not prefixed it with `xpath=` because we
internally compare the given with the generated locator.

Works: `locator('xpath=//div[contains(@Class, "foo")]')`
Does not work: `locator('//div[contains(@Class, "foo")]')`

Relates
#27707 (comment)
Fixes
microsoft/playwright-dotnet#2718 (comment)

---------

Signed-off-by: Max Schmitt <max@schmitt.mx>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Drive-by:
- extract `TestErrorView`;
- replace `data-test-id` with `data-testid` and `getByTestId()`.

---

<img width="1001" alt="top-level errors in html report"
src="https://github.com/microsoft/playwright/assets/9881434/2d6c0c52-8df1-46a9-b3fd-06ddc6f16796">
This change assumes that the user has Node 18 with Symbol.dispose
available.

Fixes #27141
@ammar-ahmed-butt ammar-ahmed-butt merged commit 559827f into ammar-knowledge:main Oct 24, 2023
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.