Optimize build matrix

[uuf6429]

Fixes #40.

GitHub workflows have some limitations on setting up "constants" or "global variables".
There are two solutions:

1. using a placeholder and then replacing it when needed with a sort of ternary operation:

   ${{ matrix.browser == 'default' && env.DEFAULT_BROWSER || matrix.browser }}

   It's quite verbose, not so readable and "default" will show up in the CI steps (not so nice).
   See run: https://github.com/minkphp/webdriver-classic-driver/actions/runs/11645388813
2. using a previous step to set up defaults, as mentioned in the documentation.
   On the whole this seems to work better and is less verbose. The extra step might be confusing and initially, it will not properly list all steps until that first one is executed.
   See run: https://github.com/minkphp/webdriver-classic-driver/actions/runs/11645451262

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.14%. Comparing base (ea2ee82) to head (12f2dc7).
Report is 5 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #41      +/-   ##
============================================
+ Coverage     81.76%   87.14%   +5.38%     
- Complexity      191      334     +143     
============================================
  Files             1        1              
  Lines           488      778     +290     
============================================
+ Hits            399      678     +279     
- Misses           89      100      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[uuf6429]

@aik099 the change in tests has revealed a "bug" in the browser name test where the input is not matching the output (chromium -> chrome and edge -> MicrosoftEdge). Not sure how to solve this TBH. :(

[aik099]

You're getting a double build count (on pull_request and on push) because you're creating branches for your PRs in the minkphp/webdriver-classic-driver repo instead of your fork repo uuf6429/webdriver-classic-driver.

Here is my setup:

Do once:

1. clone the minkphp/webdriver-classic-driver repo (that is origin remote)
2. fork the minkphp/webdriver-classic-driver repo as aik099/webdriver-classic-driver
3. add aik099 remote that points to the fork

Do each time:

1. create a PR branch
2. make code changes & commits
3. push that branch into aik099 remote with --set-upstream option so they're linked now
4. create a pull request from just pushed branch on the aik099/webdriver-classic-driver

You can see for yourself by comparing the build in mine & your PRs:

• mine: https://github.com/minkphp/webdriver-classic-driver/pull/39/checks - only pull_request part
• yours: https://github.com/minkphp/webdriver-classic-driver/pull/38/checks - both push and pull_request part
  .

[uuf6429]

You're getting a double build count (on pull_request and on push) because you're creating branches for your PRs in the minkphp/webdriver-classic-driver repo instead of your fork repo uuf6429/webdriver-classic-driver.

I was under the impression that a fork is needed for non-maintainers to contribute. As a maintainer, why should I have a fork? In that case I believe the workflow should be better adjusted.. Isn't it also more convenient for you to work on the main repo directly?

[aik099]

@aik099 the change in tests has revealed a "bug" in the browser name test where the input is not matching the output (chromium -> chrome and edge -> MicrosoftEdge). Not sure how to solve this TBH. :(

Push down the browser mapping resolution from the constructor into usage places:

1. in the driver constructor just do $this->browserName = $browserName;
2. in other places (\Mink\WebdriverClassicDriver\WebdriverClassicDriver::getBrowserSpecificCapabilities and \Mink\WebdriverClassicDriver\WebdriverClassicDriver::initCapabilities execute the self::BROWSER_NAME_ALIAS_MAP[$this->browserName] ?? $this->browserName instead of $this->browserName

This way developer will get the same browser as he provided at driver construction time, but the mapping (to satisfy WebDriver) would be purely driver internal stuff.

[uuf6429]

Out of curiosity, I checked how PHPStan does it. It looks similar to something I saw in the GH docs, wouldn't this make more sense?

on:
  pull_request:
  push:
    branches:
      - "main"

[aik099]

Out of curiosity, I checked how PHPStan does it. It looks similar to something I saw in the GH docs, wouldn't this make more sense?

on:
  pull_request:
  push:
    branches:
      - "main"

Belive me. Having PR branches on the main repo is a bad idea. Even worse these PR branches are forked to the contributors repos as well.

[aik099]

You're getting a double build count (on pull_request and on push) because you're creating branches for your PRs in the minkphp/webdriver-classic-driver repo instead of your fork repo uuf6429/webdriver-classic-driver.

I was under the impression that a fork is needed for non-maintainers to contribute. As a maintainer, why should I have a fork? In that case I believe the workflow should be better adjusted.. Isn't it also more convenient for you to work on the main repo directly?

I use the same approach for all repos regardless of the fact, that have push access to them (means I'm a maintainer there) 😄 . As I saw @stof does the same in #3 .

[aik099]

Out of curiosity, I checked how PHPStan does it. It looks similar to something I saw in the GH docs, wouldn't this make more sense?

on:
  pull_request:
  push:
    branches:
      - "main"

I'm not saying no. Let's hear other opinions: @stof , @oleg-andreyev , @mvorisek .

[mvorisek]

Thank you for asking me. I am strongly for running CI on all pushes - I very often want to run CI on non-PR branch - like before I submit PR, for experiments, ...

[uuf6429]

Thank you for asking me. I am strongly for running CI on all pushes - I very often want to run CI on non-PR branch - like before I submit PR, for experiments, ...

That sounds like you want to run it manually, right? In that case it would make sense to also keep on: workflow_dispatch.

I'm also curious to know how you handle repositories you have push access to, if you fork them or not.

[mvorisek]

I simply fork them, enable CI for that repo and push. I prefer CI on every push instead having to trigger it manually.

[aik099]

I simply fork them, enable CI for that repo and push. I prefer CI on every push instead having to trigger it manually.

The CI will be triggered manually on the origin repo as pull_request because you've pushed to the forked repo, that powers the PR itself.

@mvorisek , in this scenario you'll also get double builds:

1. when pushing to the forked repo (build in a forked repo on the push event)
2. when commit gets added to a pull request (build in origin repo on the pull_request event)

The problem, that was supposed to be solved by restricting push builds on the main branch was double builds, when maintainers create PR branches directly in the repo itself and not on their fork (see #41 (comment) ).

What do you recommend?

[mvorisek]

I am maintainer of atk4 project and we run such CI configuration for years - https://github.com/atk4/ui/blob/5.2.0/.github/workflows/test-unit.yml#L4-L7. So far the CI is great. We even run CI every 2 hours using cron, so we can easily notice dependencies issues.

I found the CI run two times is actually useful, as you can easily notice a) difference of branch vs. rebased branch on PR branch (that's how pull_request is tested), b) randomly failing tests.

[aik099]

As for the main PR topic we're all good here. Accepting changes.

As for a side topic (whatever to create PRs from branches in a main repo, that results in double builds) we can create another issue.

I personally suggest creating PRs from repo fork branches.