microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
66.5k stars 3.64k forks source link

[Bug]: Using--only-changed=$GITHUB_BASE_REF (or --only-changed=main) option to run only changed tests, results in a error in GitHub #32452

Open PramodKumarYadav opened 1 month ago

PramodKumarYadav commented 1 month ago

Version

1.46.1

Steps to reproduce

Steps:

  1. Copy the workflow from as shown in the official playwright docs from here: https://playwright.dev/docs/ci-intro#fail-fast
  2. Paste to your test repository.
  3. Change a test file.
  4. Create a pull request with this file to the main branch.

Expected behavior

This should trigger the workflow and run changed tests.

Actual behavior

We instead get this error. test

Error: The repository is a shallow clone and does not have 'main' available locally.
Note that GitHub Actions checkout is shallow by default: https://github.com/actions/checkout

Additional context

No response

Environment

System:
    OS: macOS 14.4
    CPU: (10) arm64 Apple M1 Max
    Memory: 21.78 GB / 64.00 GB
  Binaries:
    Node: 21.6.2 - /opt/homebrew/bin/node
    npm: 10.2.4 - /opt/homebrew/bin/npm
  Languages:
    Bash: 3.2.57 - /bin/bash
  npmPackages:
    @playwright/test: ^1.46.1 => 1.46.1
Skn0tt commented 1 month ago

Thanks for the report! https://github.com/microsoft/playwright/pull/32461 fixes the code snippet that you used. Let me know if it works for you if you use that updated snippet.

PramodKumarYadav commented 1 month ago

Thanks for the report! #32461 fixes the code snippet that you used. Let me know if it works for you if you use that updated snippet.

Hi SknOtt, Thanks for picking this up! I tried the change and now get another error from git as below.

Run CUSTOM_TAG_1="3" npx browserstack-node-sdk playwright test  --shard=$((0 + 1))/3  --grep="@unit-test"  --grep-invert=""  --project=chromium --only-changed=$GITHUB_BASE_REF
2024-09-05 10:54:47 - info: Reading configs from /__w/MercellForms/MercellForms/MercellForms.ComponentTests/browserstack.yml
2024-09-05 10:54:47 - info: Skipping buildIdentifier as BROWSERSTACK_BUILD_NAME is set
Error: Cannot detect changed files for --only-changed mode:
git ls-files --others --exclude-standard

fatal: detected dubious ownership in repository at '/__w/MercellForms/MercellForms'
To add an exception for this directory, call:

    git config --global --add safe.directory /__w/MercellForms/MercellForms

I added the command after checkout to see if that makes a difference.

 - name: Checkout Repository
        uses: actions/checkout@v4
        with:
          # Force a non-shallow checkout, so that we can reference $GITHUB_BASE_REF (needed for the --only-changed option to work)
          # See https://github.com/actions/checkout for more details.
          fetch-depth: 0

      - name: Configure Git safe directory
        run: git config --global --add safe.directory /__w/MercellForms/MercellForms

But it didn't.

I continue getting the error. Can someone try the change locally and see if it works on their machine?

Skn0tt commented 1 month ago

Hmm, haven't seen that error before. Stackoverflow gives me this thread: https://stackoverflow.com/questions/72978485/git-submodule-update-failed-with-fatal-detected-dubious-ownership-in-reposit Any chance you're using submodules?

PramodKumarYadav commented 1 month ago

Hmm, haven't seen that error before. Stackoverflow gives me this thread: https://stackoverflow.com/questions/72978485/git-submodule-update-failed-with-fatal-detected-dubious-ownership-in-reposit Any chance you're using submodules?

I indeed was testing in a monorepo that contains frontend and backend as sub directories in it. I assume that would be a common use case for monorepos?

That said, I tried it on a simple playwright project where the test are directly in the repository and still see failures (although with different reasons). The pipeline run and its yaml file are here: https://github.com/PramodKumarYadav/playwright-sandbox/actions/runs/10719389760/job/29723351599

1s
Run npx playwright test --only-changed=$GITHUB_BASE_REF

Running in CI
Error: Cannot detect changed files for --only-changed mode:
git diff main --name-only
fatal: ambiguous argument 'main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
Error: Error: Cannot detect changed files for --only-changed mode:
git diff main --name-only

fatal: ambiguous argument 'main': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]'

Error: Process completed with exit code 1.

Skn0tt commented 1 month ago

I indeed was testing in a monorepo that contains frontend and backend as sub directories in it.

Thanks for confirming! I'd assume that the --only-changed implementation is tripping up on the submodules, and maybe it doesn't know what module it should use for comparing? You should be able to reproduce this locally on your machine by running with --only-changed=main. I can't give advice for your specific setup because I don't have it locally, but if you could provide a minimal reproduction case I can take a look.

That said, I tried it on a simple playwright project where the test are directly in the repository and still see failures

Hmm, this is surprising! I'll take a look.

Skn0tt commented 1 month ago

I took a look at your repository. Interestingly, $GITHUB_BASE_REF seems to be main, which isn't available as a local branch with how @action/checkout@v4 sets up the repository. If you specify the remote where the branch is located (origin in most cases), things work.

Try passing --only-changed=origin/$GITHUB_BASE_REF.

PramodKumarYadav commented 1 month ago

Saw your comment now. Testing now...

PramodKumarYadav commented 1 month ago

I took a look at your repository. Interestingly, $GITHUB_BASE_REF seems to be main, which isn't available as a local branch with how @action/checkout@v4 sets up the repository. If you specify the remote where the branch is located (origin in most cases), things work.

Try passing --only-changed=origin/$GITHUB_BASE_REF.

Hi @Skn0tt,

Test 1 : On a standard setup test repository. ✅

I tested the changes with origin/$GITHUB_BASE_REF from the sandbox repository and it worked fine. So the fix did the job. Reference here: https://github.com/PramodKumarYadav/playwright-sandbox/actions/runs/10724176365/job/29739220402

Test 2: On a monorepo ❌

I cant share the project pipeline but this one still failed with the same "dubious ownership" errors as before.

Skn0tt commented 1 month ago

Good to know that it helps on a standard repo. For the monorepo, I totally understand you can't share the project. Could you try reproducing it on a separate repo, and share that with me? Would love to take a look and see what we can do.

PramodKumarYadav commented 1 month ago

Sure. If you can wait a bit, I can try it today or over the weekend and share the results with you?

PramodKumarYadav commented 1 month ago

Good to know that it helps on a standard repo. For the monorepo, I totally understand you can't share the project. Could you try reproducing it on a separate repo, and share that with me? Would love to take a look and see what we can do.

@Skn0tt : I was able to reproduce the behaviour in monorepo and I think it has got nothing to do with monorepo but running tests on a docker container with --only-changed flag.

I created a monorepo and tested the flag as you can see here: https://github.com/PramodKumarYadav/monorepo/actions

The top run is installing dependencies in the runner and running it from there and the 3rd run is running tests from docker container which throws the dubious ownership error.

image image

The PR for testing both these pipelines is here: https://github.com/PramodKumarYadav/monorepo/pull/1/files

Hope this helps isolating and finding the root cause/fix for this issue?

Skn0tt commented 1 month ago

Thanks for the repro, this was super helpful! I SSH-ed into the execution and took a look. It seems that the "dubious ownership" error is triggered by the .git directory being owned by a different Unix user than the cloned repository. This blog post explains that better. Because Docker is used here, .git is owned by root but the repo is owned by 127, which I believe is the user that owns Docker mounts.

The fix is to do what the command says, and execute git config --global --add safe.directory '*'. I'll discuss with the team later today if we can add this to the default config for our Playwright Docker image.

PramodKumarYadav commented 1 month ago

Thanks for the repro, this was super helpful! I SSH-ed into the execution and took a look. It seems that the "dubious ownership" error is triggered by the .git directory being owned by a different Unix user than the cloned repository. This blog post explains that better. Because Docker is used here, .git is owned by root but the repo is owned by 127, which I believe is the user that owns Docker mounts.

The fix is to do what the command says, and execute git config --global --add safe.directory '*'. I'll discuss with the team later today if we can add this to the default config for our Playwright Docker image.

@Skn0tt : This did the job. We are good and green in both pipelines (when running inside container and running directly in the runner): Here are the latest runs:

image

P.S. I had infact tried fixing the run by adding git config --global --add safe.directory '*' before but then since I missed to fetch git history using fetch-depth image

and missed to add origin in the target before $GITHUB_BASE_REF, it didnt work. image

Now with all these 3 changes, it works as expected :).

Thanks for all your help in resolving this. I appreciate it :).

PramodKumarYadav commented 1 month ago

@Skn0tt : One last tip from my experiments that may be useful in documentation.

image

Skn0tt commented 1 month ago

We've investigated a bit more, and this seems to happen for all Actions workflows that use containers. https://github.com/actions/runner/issues/2033 tracks this, and https://github.com/actions/runner/issues/2033#issuecomment-1598547465 seems to be another good workaround.

I'll leave this issue open and mark it as "Collecting Feedback". Let's collect upvotes on this comment from everyone who runs into this. If a lot of people see this, we can see if we can add a workaround on our end. Until then, let's hope the upstream issue gets resolved.