-
Notifications
You must be signed in to change notification settings - Fork 24
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 test #51
Playwright test #51
Conversation
Signed-off-by: Philip-21 <[email protected]>
@@ -0,0 +1,27 @@ | |||
name: Playwright Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already present in master: https://github.com/layer5labs/meshmap-snapshot/blob/master/.github/workflows/playwright.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yh i observed that after i pushed, my master branch wasnt up to-date
Awesome work @Philip-21. Please rebase your branch on master so that you don't re-add the files already present in master. |
On a side note, adding a slightly more descriptive commit message and description is always helpful for fellow developers :) |
Sure, the next commit will have it. |
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
@VaibhavMalik4187 i created new functions in the playwrightSetup file to be called in the playwright test file |
@Philip-21 you're making great progress. If you could just implement the helper functions used in Ultimately, this request includes the implementation playwright version of the following cypress functions:
I'll need to spend some more time with playwright before I become able to convert the following functions.
Feel free to ping me in case you need help with anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leecalcote, tried to address some overall Maybe problems related to JavaScript, not MeshMap or Playwright.
I'm not sure about the working of meshmap. So, sorry @Philip-21 if I'm wrong :3
path: "/api/pattern*", | ||
alias: "designEp", | ||
wait: "@designEp", | ||
absolutePath: "/api/pattern" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is api/pattern
an endpoint?
What's the use of designEndpoint
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Philip-21, may you answer this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its the ApiEndpoint for creating or deleting meshmap design .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Id added after it will point to a particular design, r8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am thinking of writing individual tests for every action but i dont think it will work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am trying to see how i ca put everything together, for it to work
// Capture a screenshot and save it with a custom filename | ||
const date = new Date(); | ||
// Format: MeshMap-Fri Sep 09 2023-10:30:45 AM.png | ||
const fileName = `MeshMap-${date.toDateString()}-${date.toLocaleTimeString()}.png`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toDateString()
will give a string with some spaces in it, r8?
The same goes for toLocaleTimeString
.
Can we format it to YYYYMMDDHHMMSS or related formats which is less ambiguous? Is it possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a way. My js isnt that solid but i'll fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const getFormattedDateTime = () => {
const now = new Date();
// Extract date components (YYYY_MM_DD format)
const year = now.getFullYear();
// Add 1 because months are 0-indexed
const month = (now.getMonth() + 1).toString().padStart(2, "0");
const day = now.getDate().toString().padStart(2, "0");
// Extract time components (HH_MM_SS format)
const hours = now.getHours().toString().padStart(2, "0");
const minutes = now.getMinutes().toString().padStart(2, "0");
const seconds = now.getSeconds().toString().padStart(2, "0");
// Concatenate date and time components into the desired format
const formattedString = `Meshmap_${year}_${month}_${day}_${hours}_${minutes}_${seconds}`;
return formattedString;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright @VaibhavMalik4187 thanks , i'll be intouch |
Signed-off-by: Philip-21 <[email protected]>
@VaibhavMalik4187 i just implemented some new functions to be used for |
|
||
//utility func to generate css selector and return id and strings | ||
function waitFor(str) { | ||
return `@${str}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if using $ to access the element is recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its the same method as return "@" + str;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${}
makes it easier to include variables and expressions within a string without explicit concatenation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think of the login and interceptRequests functions
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
I have been able to write and run some individual tests in the playWrightUtils.js The main thing is to configure meshmap snapshots to run successfully, I looked through the cypress-action files to see how that was achieved, I observed cypress has alot of inbuilt functionalities which playwright doesn't. So, I imitated some cypress commands in the commands.js and helpers.js and I defined some of these comands in the playwrightsetup.JS The constant.js which is similar to the cypress action folder , is for getting the meshmap designs and its api endpoints. these are just the progress i have made so far The approach used to run the cypress action, I feel it was a different approach as I defined some setup functions that would used to run the e2e test as the way it was implemented for the cypress-action. But I feel using the |
const signInButton = await page.waitForSelector('button[type="submit"]'); | ||
await signInButton.click(); | ||
// Wait for some time (e.g., 5 seconds) for any post-login actions or page navigation to complete | ||
await page.waitForTimeout(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Philip-21 "Never wait for timeout in production. Tests that wait for time are inherently flaky. Use Locator actions and web assertions that wait automatically."
For reference, see https://playwright.dev/docs/api/class-page#page-wait-for-timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A solid observation. 🙌
|
||
export async function MeshMap(page){ | ||
await page.goto('https://playground.meshery.io/extension/meshmap'); | ||
const meshMapBetaHeading = page.locator('h1:has-text(/MeshMap\s*beta/i)'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't page.locator('h1:has-text(/MeshMap\s*beta/i)');
missing an await
in the beginning?
// Use page.locator() to describe the headings under "MeshMap beta." | ||
const designsHeading = page.locator('h1:has-text(/MeshMap\\s*beta/i) + h2:has-text("Designs")'); | ||
const componentsHeading = page.locator('h1:has-text(/MeshMap\\s*beta/i) + h2:has-text("Components")'); | ||
const applicationsHeading = page.locator('h1:has-text(/MeshMap\\s*beta/i) + h2:has-text("Applications")'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
several of these other locator methods are also missing an await in the beginning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if those await
aren't implicit, then this certainly seems like a best practice to institute.
Signed-off-by: Philip-21 <[email protected]>
@MarioArriaga92 thanks i've fixed them |
Signed-off-by: Philip-21 <[email protected]>
This is some inspiring work, @Philip-21 😄 |
lol thanks |
@Philip-21 any idea why all the PR Check failures? |
@Philip-21 also, about the "tests_e2e" job, 2 questions/observations:
|
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
This is quite interesting, @Philip-21. Cool. |
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Signed-off-by: Philip-21 <[email protected]>
Hello @Philip-21 The workflow is You can merge your branch into your local fork and see the running tests/debug from there. |
Signed-off-by: Philip-21 <[email protected]>
…p-snapshot into playwright-test
Does it work? |
All Playwright tests work perfectly, but the workflow is still detecting
failed cypress actions. I am liaising with Abhi on this issue
…On Wed, Oct 18, 2023 at 6:26 PM Lee Calcote ***@***.***> wrote:
Does it work?
—
Reply to this email directly, view it on GitHub
<#51 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ARP2QJWIUJRXCGB7GCFDABLYAAGLTAVCNFSM6AAAAAA4SNZOSGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRZGAYDSMJZGE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Notes for Reviewers
This PR fixes #