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

e2e: test NEO session launcher and imageList #2665

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

gahyuun
Copy link
Member

@gahyuun gahyuun commented Aug 28, 2024

This PR resolves #2657 issue

TL;DR

e2e test for NEO session creation and imageList

What changed?

Test:

  • create and terminate a session without setting anything except the session name.
  • rendering image list
  • install image
  • modify image resource limit
  • manage apps

Copy link

graphite-app bot commented Aug 28, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the effort:normal Need to understand a few modules / some extent of contextual or historical information. label Aug 28, 2024
@github-actions github-actions bot added the size:L 100~500 LoC label Aug 28, 2024
@gahyuun gahyuun force-pushed the test/neo-session-and-imagelist branch from 9d95fd2 to 8764ac7 Compare August 28, 2024 09:23
e2e/test-util.ts Outdated
await page.getByRole("button",{name:"Start"}).click();

// Wait for a session to be launched
await page.waitForTimeout(10000);
Copy link
Member Author

Choose a reason for hiding this comment

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

It was difficult to me that the sessions created were not immediately reflected on the screen.

So I tried a few things

  1. refresh ( use navigateTo) -> Not suitable for user scenario testing.
  2. “Create session with session name e2e test -> Recreate session with session name e2e test. Check the failure statement that a session has already been created”
    I tried to run the test with the above scenario. But It isn't a good fit for user scenario testing either.
  3. Increasing the waitForTimeout time

I thought option 3 works best and I applied that method

Copy link
Member

Choose a reason for hiding this comment

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

Instead of page.waitForTimeout(10000), we can use locator's wait. Please check my recent commit.

await expect(session).toBeVisible({timeout: 10000});

Copy link
Member Author

Choose a reason for hiding this comment

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

I check it. Create session returns a passed. But session test fails. Because the timeout (await page.waitForTimeout(5000);)was removed and the session termination was not immediately reflected on the screen.
So I think that Delete session test needs Timeout

@gahyuun gahyuun changed the title e2e: test neo-session-creation and imageList e2e: test NEO session creation and imageList Aug 28, 2024
@gahyuun gahyuun changed the title e2e: test NEO session creation and imageList e2e: test NEO session launcher and imageList Aug 28, 2024
Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

Please check my comments and commits.

e2e/environment.test.ts Outdated Show resolved Hide resolved
e2e/environment.test.ts Outdated Show resolved Hide resolved
e2e/environment.test.ts Outdated Show resolved Hide resolved
@gahyuun gahyuun requested a review from yomybaby August 29, 2024 05:21
Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

I pushed a commit please check it.
Now, LGTM! Thank you for your contribution.

Copy link

graphite-app bot commented Aug 30, 2024

Merge activity

### This PR resolves [#2657 ](#2657) issue
### TL;DR

e2e test for NEO session creation and `imageList`

### What changed?
Test:
 - create and terminate a session without setting anything except the session name.
 - rendering image list
 - install image
 - modify image resource limit
 - manage apps
@yomybaby yomybaby force-pushed the test/neo-session-and-imagelist branch from 630ca50 to 11ca13f Compare August 30, 2024 08:30
@graphite-app graphite-app bot merged commit 11ca13f into main Aug 30, 2024
5 checks passed
@graphite-app graphite-app bot deleted the test/neo-session-and-imagelist branch August 30, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort:normal Need to understand a few modules / some extent of contextual or historical information. size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing ImageList and NEO session creation with Playwright
2 participants