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

Basic test verifying if demo account is working properly #8442

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

BOHEUS
Copy link
Contributor

@BOHEUS BOHEUS commented Nov 8, 2024

No description provided.

@BOHEUS BOHEUS marked this pull request as draft November 8, 2024 20:34
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

Added new E2E test configuration and implementation to verify the demo account functionality and server availability status.

  • Added new test project 'Demo check' in /packages/twenty-e2e-testing/playwright.config.ts for demo-specific testing
  • Created /packages/twenty-e2e-testing/tests/demo_basic.spec.ts to verify demo server status with '@demo-only' tag
  • Test expects 'Server's on a coffee break' message with 10s timeout
  • Hardcoded demo URL 'https://demo.twenty.com/' should be moved to environment configuration
  • Special character encoding issue in error message text needs fixing ('â' in "Serverâs")

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

packages/twenty-e2e-testing/tests/demo_basic.spec.ts Outdated Show resolved Hide resolved
packages/twenty-e2e-testing/tests/demo_basic.spec.ts Outdated Show resolved Hide resolved
@BOHEUS
Copy link
Contributor Author

BOHEUS commented Nov 8, 2024

It needs small fix as right now when running this test from CLI, it launches additional test used by other tests when it shouldn't as there's no need to save the state of page after authorization.

@BOHEUS BOHEUS marked this pull request as ready for review November 9, 2024 10:30
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

(updates since last review)

Reorganized test files structure and improved test configurations for better maintainability and organization.

  • Moved test files into /tests/all directory with proper test matching patterns in playwright config
  • Added explicit testMatch patterns for chromium/firefox in playwright.config.ts targeting /all/.+\.spec\.ts/
  • Hardcoded test data in /tests/all/companies.spec.ts needs refactoring (13 rows count)
  • Empty test description in companies spec needs proper naming and documentation

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

packages/twenty-e2e-testing/tests/demo/demo_basic.spec.ts Outdated Show resolved Hide resolved
await page.getByRole('button', { name: 'Continue', exact: true }).click();
await page.getByRole('button', { name: 'Sign in' }).click();
await expect(page.getByText('Welcome to Twenty')).not.toBeVisible();
await expect(page.getByText('Server’s on a coffee break')).toBeVisible({
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Special quote character 's is used instead of standard apostrophe. This may cause issues with string matching.

Comment on lines +7 to +9
await page.getByRole('button', { name: 'Continue With Email' }).click();
await page.getByRole('button', { name: 'Continue', exact: true }).click();
await page.getByRole('button', { name: 'Sign in' }).click();
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: No input validation or error handling for failed button clicks

@FelixMalfait
Copy link
Member

FelixMalfait commented Nov 12, 2024

Hey @BOHEUS thanks for this! I'm not sure we want to check external urls in a test like that. I think tests like that are meant to be able to run in an isolated environment. Otherwise this could break for things not related to the code. Tests are meant to help prevent error during deployment/PR but this is more of a reactive alert type of thing which should be done some place else. If we want to test demo/prod urls there are other QA tools we could probably use.

@FelixMalfait
Copy link
Member

FelixMalfait commented Nov 12, 2024

I discussed with @charlesBochet and he doesn't agree with my comment. Could you try creating the github action so what what it'd look like? Maybe a cron running once a day that would send an email for example? Maybe we should add a Discord alert step https://github.com/marketplace/actions/action-notify

@BOHEUS
Copy link
Contributor Author

BOHEUS commented Nov 12, 2024

No problem, I'll take care of it in free time, I'll focus on creating basic Github workflow with cron to run it once/twice per day and later I'll add additional features like email or Discord alert depending on needs

@BOHEUS
Copy link
Contributor Author

BOHEUS commented Nov 22, 2024

@charlesBochet I'm sorry I didn't inform earlier that this PR is ready to review, please check this workflow as I'm not entirely sure if that's a correct approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants