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.

Sign up for GitHub

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

Jump to bottom

[tests-only][full-ci]Cache playwright chromium #11958

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[tests-only][full-ci]Cache playwright chromium #11958

wants to merge 5 commits into from

Conversation

amrita-shrestha
Copy link
Contributor

@amrita-shrestha amrita-shrestha commented Nov 25, 2024 •
edited
Loading

Description

This PR

  • update PLUGINS_S3 = "plugins/s3:1.5" version
  • cache chromium with respect to playwright version
  • Use cached chromium to run e2e tests
  • add script to get playwright version to store cache

Note

  • can't upgrade minio version because some command are not available in the latest minio image
  • if we are covering multiple browser caching, different approach would be used but this PR only cover for chromium not every browser. The CI pipeline has been designed with simple desgin to avoid unnecessary creation of dirs and step pipeline.

Related Issue

Motivation and Context

How Has This Been Tested?

  • CI

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests
  • Documentation
  • Maintenance (e.g. dependency updates or tooling)

Open tasks:

  • ...

@amrita-shrestha amrita-shrestha self-assigned this Nov 25, 2024
@amrita-shrestha amrita-shrestha force-pushed the cache-playwright-chromium branch 13 times, most recently from b8ec784 to 6473018 Compare November 26, 2024 12:51
@amrita-shrestha amrita-shrestha force-pushed the cache-playwright-chromium branch 2 times, most recently from 31a1f65 to fa7388c Compare November 28, 2024 05:30
@amrita-shrestha amrita-shrestha marked this pull request as ready for review November 28, 2024 05:38
saw-jan
saw-jan reviewed Nov 28, 2024
View reviewed changes
.drone.star Outdated Show resolved Hide resolved
.drone.star Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 28, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

saw-jan
saw-jan reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@saw-jan saw-jan left a comment

Choose a reason for hiding this comment

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

Consider using browsers instead of chromium

.drone.star
@@ -39,6 +39,7 @@ dir = {
"ocisRevaDataRoot": "/srv/app/tmp/ocis/owncloud/data/",
"federatedOcisConfig": "/var/www/owncloud/web/tests/drone/config-ocis-federated.json",
"ocmProviders": "/var/www/owncloud/web/tests/drone/providers.json",
"chromiumZip": "/var/www/owncloud/web/playwright-chromium.tar.gz",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"chromiumZip": "/var/www/owncloud/web/playwright-chromium.tar.gz",
"playwrightBrowsersArchive": "/var/www/owncloud/web/playwright-browsers.tar.gz",

Copy link
Contributor Author

@amrita-shrestha amrita-shrestha Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What's the problem with chromium? I don't see being specific a pblm playwrightChromiumArchive than playwrightBrowsersArchive. IMO browser denotes multiple browser if we zip multiple browser than it sound good but i don't see any pblm uysing chromium name this comment is reply for overall comment. Can you avoid creating crowd while commenting? similar thing can be cover in one comment

Copy link
Member

@saw-jan saw-jan Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the archive is of a folder where browsers are installed by playwright not only chromium
Should we create another path entry for other browsers? like playwright-firefox.tar.gz

.drone.star
"image": OC_CI_NODEJS,
"environment": {
"PLAYWRIGHT_BROWSERS_PATH": ".playwright",
"PLAYWRIGHT_BROWSERS_PATH": ".chromium",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"PLAYWRIGHT_BROWSERS_PATH": ".chromium",
"PLAYWRIGHT_BROWSERS_PATH": ".playwright",

.drone.star
@@ -716,15 +723,16 @@ def installPnpm():
],
}]

def installPlaywright():
def installChromium():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def installChromium():
def installBrowsers():

.drone.star
return [{
"name": "playwright-install",
"name": "chromium-install",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "chromium-install",
"name": "install-browsers",

.drone.star
@@ -1899,3 +1910,51 @@ def getOcislatestCommitId(ctx):
],
},
]

def cacheChromium():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def cacheChromium():
def cacheBrowsers():

.drone.star

def checkChromiumCache():
return [{
"name": "check-chromium-cache",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "check-chromium-cache",
"name": "check-browsers-cache",

.drone.star
],
}]

def restoreChromiumCache():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def restoreChromiumCache():
def restoreBrowsersCache():

.drone.star
def restoreChromiumCache():
return [
{
"name": "restore-chromium-cache",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "restore-chromium-cache",
"name": "restore-browsers-cache",

.drone.star
],
},
{
"name": "unzip-chromium-cache",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "unzip-chromium-cache",
"name": "unzip-browsers-cache",

tests/drone/script.sh
}

# Function to check if the cache exists for the given commit ID
check_chromium_cache() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
check_chromium_cache() {
check_browsers_cache() {

@ScharfViktor
Copy link
Contributor

thank you Amrita, that is great job 👍
I'm fine with chromium naming if we use only chromium. when we use multi-browser testing in the CI -> we can use abstract name IMHO
I have questions:

  • do we know how long we keep data in the MINIO_MC? what happens if we don't update playwright in for example 30 days and cache will be deleted? should we check that chromium cache is exist?
  • in your PR we install chromium https://drone.owncloud.com/owncloud/web/49324/5/5 but we don't update playwright. it happened since we didn't have the cache, correct? in normal case we don't need install chromium when no playwright update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saw-jan saw-jan saw-jan left review comments

@ishabaral ishabaral Awaiting requested review from ishabaral

@Salipa-Gurung Salipa-Gurung Awaiting requested review from Salipa-Gurung

@ScharfViktor ScharfViktor Awaiting requested review from ScharfViktor

@nirajacharya2 nirajacharya2 Awaiting requested review from nirajacharya2

@anon-pradip anon-pradip Awaiting requested review from anon-pradip

@PrajwolAmatya PrajwolAmatya Awaiting requested review from PrajwolAmatya

At least 1 approving review is required to merge this pull request.

Assignees

@amrita-shrestha amrita-shrestha

Labels
QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[QA] download chromium in Ci only after updating playwright
3 participants
@amrita-shrestha @ScharfViktor @saw-jan