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

Speed up github actions for PRs #7250

Merged
merged 20 commits into from
Aug 25, 2023
Merged

Speed up github actions for PRs #7250

merged 20 commits into from
Aug 25, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Aug 24, 2023

This PR splits up the GitHub actions a bit so we can be more granular (and therefore faster) on CI in PRs.

  • Most PRs do not need to run across Windows/OSX - so by default, PRs will only run Ubuntu/Chromium test suites (test-pr-ubuntu.yml)
  • PRs touching create-remix or remix-dev are the most susceptible to Windows/OSX issues so when files in those packages are touched we also run the tests for Windows/OSX (test-pr-windows-macos.yml)
  • main/dev branches should still have the full suite run against them (test-full.yml)

@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2023

⚠️ No Changeset found

Latest commit: 68c7bfb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@brophdawg11 brophdawg11 added github_actions Pull requests that update GitHub Actions code and removed CLA Signed labels Aug 24, 2023
@brophdawg11
Copy link
Contributor Author

brophdawg11 commented Aug 24, 2023

Need to dig in and see why a bunch of expected things still say Waiting for a runner to pick up this job...

Update: It was because the os input only required a singular value but I was passing a stringified array, which is only needed for node_version/browser

@MichaelDeBoey
Copy link
Member

@brophdawg11 You probably also need to update the required workflows for PRs, ... in the repo settings

@@ -26,7 +26,7 @@ const config: PlaywrightTestConfig = {
use: devices["Desktop Safari"],
},
{
name: "edge",
name: "msedge",
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 changed the name of this profile to match the name of the browser (like the others) so we can use this name to only install the required browser during CI with:

npx playwright install --with-deps ${{ matrix.browser }}

That fails for edge because playwright refers to it as msedge

.github/workflows/test-full.yml Outdated Show resolved Hide resolved
@brophdawg11
Copy link
Contributor Author

brophdawg11 commented Aug 24, 2023

Looks like something about the npx playwright install --with-deps msedge isn't working right - it's then looking for a chrome.exe file when it runs. Will dig into that tomorrow...

Screenshot 2023-08-24 at 6 27 42 PM Screenshot 2023-08-24 at 6 27 18 PM

Update: Looks like since Edge is based on Chromium it just uses that as a default which explains the issue (from node_modules/playwright-core/lib/server/deviceDescriptorsSource.json:

  "Desktop Edge": {
    "userAgent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/113.0.5672.53 Safari/537.36 Edg/113.0.5672.53",
    "screen": {
      "width": 1920,
      "height": 1080
    },
    "viewport": {
      "width": 1280,
      "height": 720
    },
    "deviceScaleFactor": 1,
    "isMobile": false,
    "hasTouch": false,
    "defaultBrowserType": "chromium"
  },

I updated it to use actual MS Edge in 07f66e4

.github/workflows/test-full.yml Outdated Show resolved Hide resolved
.github/workflows/test-full.yml Outdated Show resolved Hide resolved
.github/workflows/test-full.yml Outdated Show resolved Hide resolved
@brophdawg11
Copy link
Contributor Author

🥳 ok all are passing now, going to revert the comments that triggered the windows tests, then once merged I can update the required checks. I guess technically the Windows/OSX checks can't be "required" anymore since they won't run on most PRs. Required will be lint + build + unit test on ubuntu + remix-cla-bot

Screenshot 2023-08-25 at 12 49 46 PM

...devices["Desktop Edge"],
// Desktop Edge uses chromium by default
// https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/server/deviceDescriptorsSource.json#L1502
channel: "msedge",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brophdawg11 brophdawg11 merged commit 57c1a92 into dev Aug 25, 2023
5 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/speedy-actions branch August 25, 2023 18:15
brophdawg11 added a commit that referenced this pull request Aug 25, 2023
@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Aug 25, 2023

@brophdawg11 Required steps are now blocking PRs if docs-only PRs are made (see #7190)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants