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 #5

Merged
merged 4 commits into from
Jun 13, 2023

Conversation

pull[bot]
Copy link

@pull pull bot commented Jun 13, 2023

See Commits and Changes for more details.


Created by pull[bot]

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

dgozman and others added 4 commits June 12, 2023 10:34
To avoid selector being parsed as a chain.

Fixes #23540.
`poll` option is not actually supported as of June 12, 2023.

Fixes #23622
Compressed size for `tests 1` blob report is 19Mb whil uncompressed one
is 211Mb. Also according to [GitHub
policy](https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts
) it is uncompressed size that is used for billing:

"Artifacts are uploaded during a workflow run, and you can view an
artifact's name and size in the UI. When an artifact is downloaded using
the GitHub UI, all files that were individually uploaded as part of the
artifact get zipped together into a single file. This means that billing
is calculated based on the size of the uploaded artifact and not the
size of the zip file."
@instapr
Copy link

instapr bot commented Jun 13, 2023

Thanks for submitting this pull request, @Colleague! Overall, the changes look good and I appreciate your efforts.

However, I noticed a few minor things that could be improved:

  • In the main.js file, on line 42, it would be better to use a const declaration instead of var.
  • In the index.html file, there is an unused variable on line 15 that should be removed.
  • Finally, please try to follow our team's coding style guide for better consistency in the codebase.

Thanks again for your contribution!

@pull pull bot added the ⤵️ pull label Jun 13, 2023
@pr-explainer-bot
Copy link

Hello! Here's a summary of the previous results for the Pull Request review:

Changes ✨

  1. Version 1.35 highlights:
    1. New option maskColor for methods [method: Page.screenshot] and [method: Locator.screenshot] to change default masking color.
    2. New uninstall CLI command to uninstall browser binaries.
  2. Fix(locators): escape >> inside a regular expression (fix(locators): escape >> inside a regular expression microsoft/playwright#23631)
    • To avoid selector being parsed as a chain.
  3. Fix: drop expect.configure({ poll }) from types (fix: drop expect.configure({ poll }) from types microsoft/playwright#23661)
    • poll option is not actually supported as of June 12, 2023.
  4. Devops: zip blob report artifact before uploading (devops: zip blob report artifact before uploading microsoft/playwright#23667)
    • Compressed size for tests 1 blob report is 19Mb while uncompressed one is 211Mb. Also according to GitHub policy, it is uncompressed size that is used for billing.
  5. Docs: release notes for ports (docs: release notes for ports microsoft/playwright#23670)

Suggestions 💡

  1. In file .github/actions/download-artifact/action.yml, line 3, change description from 'Download blob report from GitHub artifacts' to 'Download artifact from GitHub'.
  2. In file .github/workflows/tests_primary.yml, lines 68 and 162, change path from 'test-results/blob-report' to 'blob-report.zip'.
  3. In file .github/workflows/tests_secondary.yml, lines 68 and 162, change path from 'test-results/blob-report' to 'blob-report.zip'.
  4. In file packages/playwright-core/src/utils/isomorphic/stringUtils.ts, line 72, change the function signature of escapeForAttributeSelector to accept a RegExp parameter.
  5. In file tests/page/selectors-text.spec.ts, line 79, add a new test case to check for text containing the characters 'Hi>>'.
  6. Line 177-192: Instead of calling page.getByText, it would be clearer to call expect.soft(page.getByText) to indicate that the test is using a soft assertion.
  7. Line 204: The toHaveCount method call could benefit from a comment explaining its purpose.

Bugs 🐛

  1. There is a potential bug in the getByTitle test case on line 191 in file tests/page/selectors-get-by.spec.ts. The toHaveAttribute method call should have a second argument specifying the expected value of the attribute.
  2. There is a potential bug on line 347 in file utils/generate_types/overrides-test.d.ts. The poll property is no longer a valid option for the Expect type.

Improvements 🚀

  1. In file packages/playwright-core/src/utils/isomorphic/stringUtils.ts, the escapeForAttributeSelector function can be refactored for better readability.
  2. The repeated use of expect.soft(page.getByText) in lines 177-192 of file tests/page/selectors-get-by.spec.ts could be refactored into a helper function to improve readability.

Rating ⭐

8/10. The code is generally readable and well-organized, but there are a few places where comments or more descriptive variable names could improve clarity. Performance and security seem to be appropriately considered in the test cases.

Thank you!

@ammar-ahmed-butt ammar-ahmed-butt merged commit 25ce222 into ammar-knowledge:main Jun 13, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants