Nala pom and tests enhancements #91

skumar09 · 2023-03-27T07:51:38Z

This PR is with respect to enhancement of Nala playwright framework and address few current issues, also provide few best practices and guidelines for tests, specs and pom.

Test Structure
In Current test structure
- we are executing all specs or testcases inside a features for-loop, with increase in specs counts it
  is getting difficult to add new test and tweak the logic, it is not scalable.
- it is also quite difficult to read, debug and understand the tests, steps and flows.
With this PR , provided the following :
- All tests are grouped under test.describe() that is something like suite ex: carousel-block test suite.
- Each spec or testcase from .spec.js file corresponds to a separate test. (as below screenshot.)
- Introduced test.step() under test()
  - This will introduce better readability, separation of code / logic for different tests and test steps
Spec
- Spec structure is simplified to the array as follows, (we can add items as required for the specs)
- Parsing Spec.js is not required.
- Env baseURL is taken from playwright.config.js. ( This part is still in enhancement )
Page Objects.
- Provided the sample for how to define selectors with reference to parent selector or element.
- Provided method, guideline, and commenting format for page object methods.
Dashboard Report Util.
- Dashboard report utility was failing when the test name is changed, ( i.e. test name was tightly coupled to have four fixed parameters or arguments, and it was failing when test name has comma separator
-
- Above issue is now fixed,
- Now test name can have comma, or space as a delimiter
- Also test name can have parameters as many as required

Features/Specs

Tests

POM

Test results

are more readable with test steps

JackySun9 · 2023-03-27T17:27:03Z

playwright.config.js

    /* Base URL to use in actions like `await page.goto('/')`. */
    // baseURL: 'http://localhost:3000',

    /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */
    trace: 'on-first-retry',
+    baseURL: 'https://main--milo--adobecom.hlx.page',


Maybe we can introduce profile concept, because the baseUrl is totally different per team

@JackySun9 : this one still work in progress,[ in the playwright world we can have multipleconfig.jsor globalsetup.js for environment, working on...]

If I'm considering this right, when Nala becomes a consumable tool teams should be able to configure the baseURL to their desired one for their team. Having so many profiles/configs will get bulky in my opinion and for core Nala, if it isn't going to be configurable I think this will become bulky and heavy on maintenance.

@skumar09 You mentioned in the PR description that Parse.js would not need to be used. The usage of baseURL, with how I understand it is for a default mostly used URL that can be used when you tell playwright's browsers to go to '/' in a page.goto() call, not as a addition to be attached to another array of path strings. Otherwise, what is the plan for tests that span over different environments for a team? I'm specifically considering Milo core block testing since Milo will need to be able to test against consumer sites. Also the parse.js was used to apply rules for when the ?milolibs parameter should be added as well as when "main" in the Franklin/Helix URL should be swapped out to a feature branch so that PR testing against specific branches could happen before merging code changes into live, i.e., main--milo--adobecom.hlx.live would be swapped out for myfeaturebranchname--milo--adobecom.hlx.live. This functionality is critical and necessary, if that is removed then we will not be supporting the needs of PR testing, which is one of the core features of Nala. If we want to remove the need for parsing then there needs to be something else in place that can be used on a core level for all consumers to be able to add the "milolibs?=" parameter against other consumer sites to ensure milo block changes doesn't break anything live on consumer sites as well as swapping out repository branches for Franklin/Helix URLs for PR testing against new code.

JackySun9 · 2023-03-27T17:29:55Z

selectors/milo/carousel.block.page.js

+/* eslint-disable import/no-import-module-exports */
+import { expect } from '@playwright/test';
+
+exports.Carousel = class Carousel {


POM is good, we might have some base pages, the purpose for this design is inheritance and reuse

JackySun9 · 2023-03-27T17:31:56Z

tests/milo/carousel.block.test.js

+    });
+
+    // test step-2
+    await test.step('Verify Carousel container ', async () => {


if define some method, can we reuse it?

JingleH

looks good to me

Dli3

LGTM.

rgclayton

Found a minor typo

rgclayton · 2023-03-28T17:35:55Z

selectors/milo/carousel.block.page.js

+  /**
+ * Click carousel <lightbox modal close> button
+ */
+  async closeLighboxModal() {


Typo. Should be closeLightboxModal

rgclayton · 2023-03-28T17:36:17Z

tests/milo/carousel.block.test.js

+      await carousel.expandLighboxModal();
+
+      expect(await carousel.isLightboxCloseButtonVisible()).toBeTruthy();
+      await carousel.closeLighboxModal();


Typo. Should be closeLightboxModal

…to carousel-block

amauch-adobe

I like adding the direction for more structured and readable test scripts by using the out-of-the-box playwright annotations, test.step() methods, etc. I think that is a great adjustment and move, I think having the baseURL configurable is the best option instead of creating more complex profiles and bulking up Nala with tons of different config files per team. I also think the baseURL is a default option to use if it works for your test, but shouldn't be always used (see my comments about PR testing, the usage of parse.js with feature branch testing, and the ?milolibs= URL parameter). I think the tcid addition to specs will also help with the TestRail integration. The POM model looks great, and fits exactly what we have already discussed with Daniel Chivesco with his PRs. I also don't want us to forget the other mission of Nala was to make it simple for all developer types and not just QEs or testers. One of the reasons for creating Nala was to bridge gaps between all team members to feel comfortable in using a tool to create End-to-End tests and to do so quickly. Whenever we add, change, or switch to a new way of doing things we need to make sure there is buy-in to the changes, don't remove key functionality that is core to Nala, and keep things simple for all developers. @skumar09, I think having a huddle on this one would be good for me so I can be on the same page for some of the changes.

amauch-adobe · 2023-03-28T18:13:07Z

features/milo/carousel.block.spec.js

+      name: '@Carousel(lightbox)',
+      path: '/drafts/nala/blocks/carousel/fullpage-carousel',
+      tags: '@carousel @carousel-container @smoke @regression @milo,',
+      envs: '@milo-live milo-prod',


missing '@' symbol in front of milo-prod for envs

amauch-adobe · 2023-03-28T18:13:19Z

features/milo/carousel.block.spec.js

+      name: '@Carousel Multi slide(show-2)',
+      path: '/drafts/nala/blocks/carousel/carousel-show-2',
+      tags: '@carousel @carousel-container @regression @milo',
+      envs: '@milo-live milo-prod',


missing '@' symbol in front of milo-prod for envs

amauch-adobe · 2023-03-28T18:25:04Z

playwright.config.js

  /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */
  use: {
    /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */
-    actionTimeout: 30000,
+    actionTimeout: 60000,


What is the logic/reason for needing an action timeout of a minute long? Just curious.

amauch-adobe · 2023-03-28T18:29:29Z

playwright.config.js

@@ -34,16 +34,17 @@ const config = {
  /* Reporter to use. See https://playwright.dev/docs/test-reporters */
  reporter: process.env.CI
    ? [['github'], ['./utils/reporters/json-reporter.js'], ['./utils/reporters/api-reporter.js']]
-    : [['html', { outputFolder: 'test-html-results' }]],
+    : [['html', { outputFolder: 'test-html-results' }], ['./utils/reporters/json-reporter.js']],


What are we using the JSON-reporter locally for? For GitHub, it is necessary so that the Nala Dashboard App can pull it down/download it for its reporting. As we move forward with reporting improvements I want to make sure we keep Jingle involved since I would hate all the work Jingle put forth per Chris/Ryan's direction and many other mangers/directors involvement for the Nala Dashboard App to become inconsequential.

amauch-adobe · 2023-03-28T18:42:45Z

playwright.config.js

    /* Base URL to use in actions like `await page.goto('/')`. */
    // baseURL: 'http://localhost:3000',

    /* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */
    trace: 'on-first-retry',
+    baseURL: 'https://main--milo--adobecom.hlx.page',


@skumar09 You mentioned in the PR description that Parse.js would not need to be used. The usage of baseURL, with how I understand it is for a default mostly used URL that can be used when you tell playwright's browsers to go to '/' in a page.goto() call, not as a addition to be attached to another array of path strings. Otherwise, what is the plan for tests that span over different environments for a team? I'm specifically considering Milo core block testing since Milo will need to be able to test against consumer sites. Also the parse.js was used to apply rules for when the ?milolibs parameter should be added as well as when "main" in the Franklin/Helix URL should be swapped out to a feature branch so that PR testing against specific branches could happen before merging code changes into live, i.e., main--milo--adobecom.hlx.live would be swapped out for myfeaturebranchname--milo--adobecom.hlx.live. This functionality is critical and necessary, if that is removed then we will not be supporting the needs of PR testing, which is one of the core features of Nala. If we want to remove the need for parsing then there needs to be something else in place that can be used on a core level for all consumers to be able to add the "milolibs?=" parameter against other consumer sites to ensure milo block changes doesn't break anything live on consumer sites as well as swapping out repository branches for Franklin/Helix URLs for PR testing against new code.

amauch-adobe · 2023-03-28T18:47:00Z

selectors/milo/carousel.block.page.js

I think it makes more sense to rename the selectors directory to "pages" since POMs are more than just selectors but also methods for the page that may not just be against the said selectors. I think we already discussed this though and decided to have them under page/, I might have forgot though. :)

amauch-adobe · 2023-03-28T19:00:31Z

tests/milo/carousel.block.test.js

+// Carousel blocks tests
+test.describe('Milo Carousel block test suite', () => {
+  // Test - 1
+  test(`${features[0].name}, @milo-live, ${features[0].tags},https://milo.adobe.com `, async ({ page, baseURL }) => {


where the envs is being added as @milo_live and @milo_prod having "https://milo.adobe.com added to the test title would make it incorrect since @milo_live is actually https://main--milo--adobecom.hlx.live. So the resulting report would provide the incorrect URL it was tested against. Also if you are adding envs as a part of the spec for environments those paths should be tested against, how does the current test setup work to test against those different environments? Currently, from what I can see this will only test against what the base URL is which may not ever be any of those environments in the spec file. As mentioned in my previous comment the parse.js lib was setup so that you could test a block against many different environments for the same path since you could run a test against hlx.page or hlx.live or a production URL with the same path. I guess looking at the current commit for the spec, we would have to add a test() for every feature spec array we make and only have one env variable for them even though the tags and paths would be the same, that is one way of getting around this problem but it also adds a bit of redundancy in feature spec.

amauch-adobe · 2023-03-28T19:01:45Z

utils/extract-test-title.js

I'm okay with the adjustments, I just want to ensure that things are still grouped correctly in the Nala Dashboard App for test results. I also want to ensure the filtering will still work. @JingleH do we know if that is the case?

I was being lazy and simply hoping to see if this breaks the dashboard or not haha. If we decide to do things this way and merge this PR, I can make corresponding changes in the dashboard if needed. Looks like it's sending all the required params, so hopefully nothing breaks. Thanks for letting me know!

amauch-adobe

Approved, had huddle with @skumar09 everything looks good, and answered concerns.

* Nala pom and tests enhancements * update spec name and url in test title to stop reporter util failure * temporary update to test title to debug reporter.js failure * temporary update to test title to debug reporter.js failure_2 * temporary update to test title to debug reporter.js failure_3 * updatating title back to first commit * fixed the reporter.js failure for new test title update * fixed the typo in lightbox method name --------- Co-authored-by: nateekar <nateekar@adobe.com> Co-authored-by: Aaron Mauchley <mauchley@adobe.com>

Nala pom and tests enhancements

5b78f09

skumar09 requested review from auniverseaway, rgclayton, Dli3, amauch-adobe and JackySun9 March 27, 2023 07:51

skumar09 added @carousel Run Carousel Block Tests Nala run-nala Run Nala Test Automation against PR/Branch labels Mar 27, 2023

nateekar added 4 commits March 27, 2023 01:29

update spec name and url in test title to stop reporter util failure

71fc966

temporary update to test title to debug reporter.js failure

2b5f480

temporary update to test title to debug reporter.js failure_2

acff675

temporary update to test title to debug reporter.js failure_3

6bca733

skumar09 removed the run-nala Run Nala Test Automation against PR/Branch label Mar 27, 2023

updatating title back to first commit

7722653

JackySun9 requested changes Mar 27, 2023

View reviewed changes

fixed the reporter.js failure for new test title update

535e3b7

skumar09 added the run-nala Run Nala Test Automation against PR/Branch label Mar 27, 2023

JackySun9 approved these changes Mar 27, 2023

View reviewed changes

Merge branch 'main' into carousel-block

aece1b5

skumar09 requested a review from JingleH March 28, 2023 17:31

JingleH approved these changes Mar 28, 2023

View reviewed changes

Dli3 approved these changes Mar 28, 2023

View reviewed changes

rgclayton reviewed Mar 28, 2023

View reviewed changes

nateekar added 3 commits March 28, 2023 10:47

fixed the typo in lightbox method name

1fefce8

Merge branch 'main' into carousel-block

68346f7

Merge branch 'carousel-block' of https://github.com/skumar09/janus in…

d29c6a0

…to carousel-block

skumar09 requested a review from rgclayton March 28, 2023 18:43

amauch-adobe requested changes Mar 28, 2023

View reviewed changes

amauch-adobe approved these changes Mar 28, 2023

View reviewed changes

rgclayton approved these changes Mar 28, 2023

View reviewed changes

skumar09 merged commit db99f16 into adobecom:main Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nala pom and tests enhancements #91

Nala pom and tests enhancements #91

skumar09 commented Mar 27, 2023 •

edited

Loading

JackySun9 Mar 27, 2023

skumar09 Mar 27, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

JackySun9 Mar 27, 2023

JackySun9 Mar 27, 2023

JingleH left a comment

Dli3 left a comment

rgclayton left a comment

rgclayton Mar 28, 2023

rgclayton Mar 28, 2023

amauch-adobe left a comment

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

amauch-adobe Mar 28, 2023

JingleH Mar 28, 2023

amauch-adobe left a comment

Nala pom and tests enhancements #91

Nala pom and tests enhancements #91

Conversation

skumar09 commented Mar 27, 2023 • edited Loading

This PR is with respect to enhancement of Nala playwright framework and address few current issues, also provide few best practices and guidelines for tests, specs and pom.

Features/Specs

Tests

POM

Test results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JingleH left a comment

Choose a reason for hiding this comment

Dli3 left a comment

Choose a reason for hiding this comment

rgclayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amauch-adobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amauch-adobe left a comment

Choose a reason for hiding this comment

skumar09 commented Mar 27, 2023 •

edited

Loading