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 test #51

Merged
merged 28 commits into from
Oct 18, 2023
Merged

Playwright test #51

merged 28 commits into from
Oct 18, 2023

Conversation

Philip-21
Copy link
Contributor

Notes for Reviewers

This PR fixes #

  • Yes, I signed my commits.

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@@ -0,0 +1,27 @@
name: Playwright Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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

@VaibhavMalik4187
Copy link
Contributor

Awesome work @Philip-21. Please rebase your branch on master so that you don't re-add the files already present in master.

@VaibhavMalik4187
Copy link
Contributor

On a side note, adding a slightly more descriptive commit message and description is always helpful for fellow developers :)
https://cbea.ms/git-commit/

@Philip-21
Copy link
Contributor Author

Philip-21 commented Sep 11, 2023

On a side note, adding a slightly more descriptive commit message and description is always helpful for fellow developers :) https://cbea.ms/git-commit/

Sure, the next commit will have it.

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

Philip-21 commented Sep 11, 2023

@VaibhavMalik4187 i created new functions in the playwrightSetup file to be called in the playwright test file
There are several commands implemented in cypres which cant be done in playwright.
looking at this file
I can implement login, Setting Viewport, making api request and intercepting requests to be used for testcases.

@VaibhavMalik4187
Copy link
Contributor

VaibhavMalik4187 commented Sep 11, 2023

@VaibhavMalik4187 i created new functions in the playwrightSetup file to be called in the playwright test file There are several commands implemented in cypres which cant be done in playwright. looking at this file I can implement login, Setting Viewport, making api request and intercepting requests to be used for testcases.

@Philip-21 you're making great progress. If you could just implement the helper functions used in beforeEachCallbackForCustomUrl, then I'll quickly raise a PR to replace cypress with playwright in meshmap-snapshot action to run the loadDesign e2e test.

Ultimately, this request includes the implementation playwright version of the following cypress functions:

  1. intercept
  2. setViewPort
  3. login

I'll need to spend some more time with playwright before I become able to convert the following functions.

  1. setReleaseTag
  2. interceptCapabilities
  3. setMode

Feel free to ping me in case you need help with anything.

Copy link

@Aritra8438 Aritra8438 left a 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"

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?

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?

Copy link
Contributor Author

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 .

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

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

Copy link
Contributor Author

@Philip-21 Philip-21 Sep 16, 2023

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`;

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?

Copy link
Contributor Author

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

Copy link
Contributor

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;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

image

@Philip-21
Copy link
Contributor Author

Philip-21 commented Sep 11, 2023

@VaibhavMalik4187 i created new functions in the playwrightSetup file to be called in the playwright test file There are several commands implemented in cypres which cant be done in playwright. looking at this file I can implement login, Setting Viewport, making api request and intercepting requests to be used for testcases.

@Philip-21 you're making great progress. If you could just implement the helper functions used in beforeEachCallbackForCustomUrl, then I'll quickly raise a PR to replace cypress with playwright in meshmap-snapshot action to run the loadDesign e2e test.

Ultimately, this request includes the implementation playwright version of the following cypress functions:

  1. intercept
  2. setViewPort
  3. login

I'll need to spend some more time with playwright before I become able to convert the following functions.

  1. setReleaseTag
  2. interceptCapabilities
  3. setMode

Feel free to ping me in case you need help with anything.

Alright @VaibhavMalik4187 thanks , i'll be intouch

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

@VaibhavMalik4187 i just implemented some new functions to be used for beforeEachCallbackForCustomUrl


//utility func to generate css selector and return id and strings
function waitFor(str) {
return `@${str}`;
Copy link
Contributor

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.

Copy link
Contributor Author

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;

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

Philip-21 commented Sep 17, 2023

I have been able to write and run some individual tests in the playWrightUtils.js
these are just tests in accessing the meshery page and they ran successfully .

meshmaptest1

The main thing is to configure meshmap snapshots to run successfully,
I am figuring out how i will configure and put things together to make it a success

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 spec.js file wasn't used to run it.
The function in the loadDesign.js runs the e2e tests as we can see in the action workflow. These are just my observations so far

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 spec.js will be better for running e2e tests for playwright, as i was able to run individual tests successfully in the spec file . I am also figuring out how to call and perform the meshMap snapshot actions with configurations in the playwrightsetup.js and the design endpoint in the
constant.js which will be called and ran in the spec file

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);
Copy link

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

Copy link
Contributor

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)');

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")');

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

Copy link
Contributor

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 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Contributor Author

@MarioArriaga92 thanks i've fixed them

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@hamza-mohd
Copy link

This is some inspiring work, @Philip-21 😄

@Philip-21
Copy link
Contributor Author

This is some inspiring work, @Philip-21 😄

lol thanks

@MarioArriaga92
Copy link

@Philip-21 any idea why all the PR Check failures?
CleanShot 2023-09-28 at 10 56 35@2x

@MarioArriaga92
Copy link

@Philip-21 also, about the "tests_e2e" job, 2 questions/observations:

  1. Is there a way to make "Run Playwright tests" step a bit more verbose so titles of passing tests are shown?
  2. seems like upload-artifact action/step isn't finding any files to upload.

See screenshot for reference:
CleanShot 2023-09-28 at 10 58 21@2x

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@hamza-mohd
Copy link

This is quite interesting, @Philip-21. Cool.

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Abhishek-kumar09
Copy link
Contributor

Hello @Philip-21 The workflow is pull_request_target, it only runs on master branch, so its obvious to fail here. Your local changes are running good?

You can merge your branch into your local fork and see the running tests/debug from there.

@Philip-21 Philip-21 mentioned this pull request Oct 18, 2023
1 task
@Philip-21 Philip-21 merged commit a96a237 into layer5labs:master Oct 18, 2023
2 of 6 checks passed
@leecalcote
Copy link
Contributor

Does it work?

@Philip-21
Copy link
Contributor Author

Philip-21 commented Oct 20, 2023 via email

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.

None yet

7 participants