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

Playwright basic utils #7930

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Conversation

BOHEUS
Copy link
Contributor

@BOHEUS BOHEUS commented Oct 21, 2024

Related to #6641

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request introduces utility functions for Playwright E2E testing, focusing on environment variables management and keyboard shortcuts handling.

  • packages/twenty-e2e-testing/drivers/env_variables.ts: Implements envVariables function, but lacks error handling and may expose sensitive data
  • packages/twenty-e2e-testing/lib/utils/keyboardShortcuts.ts: Adds cross-platform keyboard shortcut utilities, could benefit from more flexible shortcut definitions
  • packages/twenty-e2e-testing/lib/utils/pasteCodeToCodeEditor.ts: Implements pasteCodeToCodeEditor, but may need error handling for locator failures
  • packages/twenty-e2e-testing/lib/utils/uploadFile.ts: Adds fileUploader function, could be improved with timeout handling and file existence checks

4 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +11 to +13
await locator.click();
await selectAllByKeyboard(page);
await page.keyboard.type(code);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider adding a small delay between actions to mimic human behavior more closely

) => {
await locator.click();
await selectAllByKeyboard(page);
await page.keyboard.type(code);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: use page.keyboard.insertText() instead of type() for better performance with large code strings

await trigger();
const fileChooser = await fileChooserPromise;
await fileChooser.setFiles(
path.join(__dirname, '..', 'test_files', filename),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The hardcoded path to 'test_files' might not be flexible for all test setups. Consider making this configurable

Comment on lines +9 to +11
const fileChooserPromise = page.waitForEvent('filechooser');
await trigger();
const fileChooser = await fileChooserPromise;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Ensure the trigger is called before waiting for the file chooser event to prevent potential race conditions

@charlesBochet
Copy link
Member

/award 500

Copy link

oss-gg bot commented Oct 31, 2024

Awarding BOHEUS: 500 points 🕹️ Well done! Check out your new contribution on oss.gg/BOHEUS

@lucasbordeau lucasbordeau merged commit c3343d3 into twentyhq:main Nov 7, 2024
15 of 16 checks passed
@BOHEUS BOHEUS deleted the playwright-add-utils branch November 8, 2024 13:10
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