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

[MWPW-136595] Move critical FEDS block checks to Milo #180

Merged
merged 12 commits into from
Oct 9, 2023

Conversation

chadsunvice
Copy link
Contributor

This PR is related to the [Automation] Move FEDS tests to Milo folder Jira ticket.

Background: run-nala label should now also run all critical FEDS checks for critical blocks like Header, Footer, Consent, Breadcrumbs (more in the future).

Thus, we moved the FEDS checks inside the Milo test folder. Page-objects (/selectors/feds) will be imported from FEDS folder and not moved to the Milo branch.

❗ We have excluded the non-critical flows, as well as the User Profile flows in hopes of not blocking Milo PRs when there are issues with missing login credentials.

@skumar09
Copy link
Contributor

skumar09 commented Sep 28, 2023

@chadsunvice: I think the following would be a quick and simple approach for running feds test scripts on pr-branch or with the run-nala label on Milo PRs

  1. Provide the path of the feds test script in testDir in root playwright.config.js ( ex: testDir: ['./test/milo', '../test/feds', ]

Other thoughts...

  • Also, if we can move the test pages to the Nala draft folder, that would be great (/libs/feds/drafts/qa/ To /drafts/nala/blocks/
  • In my last PRs, I felt having tcid / test case id in .spec.js is really helpful, as I was trying to fix aside scripts, and it took a bit of time to map the test case from .spec.js to test in test.js
  • And there are a few failures in feds tests. Can you review them? ( I tried running locally also.)

@skumar09 skumar09 added the run-nala Run Nala Test Automation against PR/Branch label Sep 28, 2023
Copy link
Collaborator

@amauch-adobe amauch-adobe left a comment

Choose a reason for hiding this comment

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

Approved.

@chadsunvice
Copy link
Contributor Author

@chadsunvice: I think the following would be a quick and simple approach for running feds test scripts on pr-branch or with the run-nala label on Milo PRs

  1. Provide the path of the feds test script in testDir in root playwright.config.js ( ex: testDir: ['./test/milo', '../test/feds', ]

Other thoughts...

The approach of only updating the testDir would be beautiful, as it would only force us to separate the FEDS features into Milo-critical and non-Milo-critical. Alas, from my research, testDir doesn't allow anything other than a string at the current state of the framework. This feature has been proposed, but unfortunately it looks as though the core-team didn't think it was worth their time.

Link: microsoft/playwright#7403 (comment)

Moreover, please consider that if this worked, we still had to split our test-cases in two, since we aren't running all FEDS-specific cases (the ones still in tests/feds/). Those are FEDS-specific and not in the direct scope of Milo logic.

  • Also, if we can move the test pages to the Nala draft folder, that would be great (/libs/feds/drafts/qa/ To /drafts/nala/blocks/

In the future, we plan on doing this. I don't think this PR will accommodate this though. There are underlying things to consider too.

  • In my last PRs, I felt having tcid / test case id in .spec.js is really helpful, as I was trying to fix aside scripts, and it took a bit of time to map the test case from .spec.js to test in test.js

For me personally, this is flavour since I already mentioned my concerns in feature test/spec files where a lot of tests will suffer modifications and the feature might end up useless. The indexing will be pretty lax, because some cases might get commented-out with time, while others will be added, and the hassle of re-indexing will still be there.

  • And there are a few failures in feds tests. Can you review them? ( I tried running locally also.)

This is a very valid point and I'm not closing this PR until I figure out which and why they are failing.

@chadsunvice
Copy link
Contributor Author

After investigating it looks like failing checks rely on having the GDPR consent visible and interacting with it. Will update our checks to force the consent modal when ran on US servers.

❗ Don't merge before we fix all failures as this will introduce delays in Milo PR approvals ❗

@chadsunvice
Copy link
Contributor Author

I have run tens of regressions on this PR and I can say that I stabilised it, but it is in no way 100% accurate. This is because of the <object>.<method>: Target closed error, which plagues the Github Actions runs. On further research, I've seen this is a regular issue, especially on webkit runs. I can only suggest that people having issues with the Milo PR approval, RE-RUN the checks until they pass.

More information here: microsoft/playwright#13090

I will attach a last screenshot of the Run-Nala regression.
Screenshot 2023-10-09 at 17 10 33

Cc: @amauch-adobe , @skumar09 , @narcis-radu .

@chadsunvice chadsunvice merged commit b9a1d19 into adobecom:main Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-nala Run Nala Test Automation against PR/Branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants