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

The README.md improvements #13

Merged
merged 3 commits into from
Feb 26, 2024
Merged

Conversation

aik099
Copy link
Member

@aik099 aik099 commented Feb 25, 2024

Addressed issues:

  • the driver usage example PHP code is missing -> added an example;
  • the Selenium 2 version is used -> using Selenium 3 version.

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.54%. Comparing base (ce10226) to head (3399c07).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main      #13   +/-   ##
=========================================
  Coverage     83.54%   83.54%           
  Complexity      191      191           
=========================================
  Files             1        1           
  Lines           468      468           
=========================================
  Hits            391      391           
  Misses           77       77           

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

Copy link
Member

@uuf6429 uuf6429 left a comment

Choose a reason for hiding this comment

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

Some side notes:

  1. We could also update the example selenium version to 4, since this driver actually supports it.

  2. I'm a fan of running README code in tests, to ensure that that code remains executable and up-to-date with our changes. If we can agree on this, I can create an issue to set that up later.

    In theory it would look like:

    ```php
    echo 'Hello world';
    
    // Outputs:
    // Hello world
    ```
    /**
     * @dataProvider readmeExamplesDataProvider
     */
    public function testThatExampleWorks(string $lang, string $input, string $output): void
    {
        switch ($lang) {
            case 'php':
                $this->expectOutputString($output);
                eval($input);
                return;
    
            // ...
        }
    }

    (readmeExamplesDataProvider parses the readme and would return ['php', "echo 'Hello world';", 'Hello world'])

After you decide on those two points, feel free to merge this PR.

@AlexSkrypnyk
Copy link

Could you please also add a section that would explain the difference between this project and minkphp/MinkSelenium2Driver because it is quite hard to understand what "classic" means exactly and what the roadmap looks like.

I'm coming from Drupal and Behat, and Mink driver is not directly used, so I'm just trying to wrap my head around what this classic driver will replace and whether that replacement be a drop-in or if we need to re-write some other system.

@aik099
Copy link
Member Author

aik099 commented Feb 26, 2024

We could also update the example selenium version to 4, since this driver actually supports it.

@uuf6429 , done.

I'm a fan of running README code in tests, to ensure that that code remains executable and up-to-date with our changes. If we can agree on this, I can create an issue to set that up later.

@uuf6429 , Not sure how you can verify, that code actually works considering that:

  1. we're area using a non-existing URL in the example;
  2. the actions we're doing on the page also are non-real;
  3. executing example code from within a test suite isn't the same as the real user is executing it (standalone PHP file), where no classes are auto-loaded yet.

Could you please also add a section that would explain the difference between this project and minkphp/MinkSelenium2Driver because it is quite hard to understand what "classic" means exactly and what the roadmap looks like.

@AlexSkrypnyk , here you go:

minkphp/MinkSelenium2Driver

This is the Mink driver developed for Selenium 2, but also works on Selenium 3 (no window/frame switching support). There are PRs opened for fixing window/frame switching ability. It can't support Selenium 4, because it uses a different API. Internally it uses https://github.com/instaclick/php-webdriver to communicate with the Selenium server.

minkphp/webdriver-classic-driver

This is the Mink driver developed from scratch for Selenium 2 and up (including Selenium 3, and Selenium 4) support using https://github.com/php-webdriver/php-webdriver to communicate with the Selenium server. IMO the classic term was used to avoid using the Selenium server version in the repository name.

I'm coming from Drupal and Behat, and Mink driver is not directly used, so I'm just trying to wrap my head around what this classic driver will replace and whether that replacement be a drop-in or if we need to re-write some other system.

@AlexSkrypnyk , currently there is no way to use this driver with Behat (there is an #15 issue about this) and once implemented using this driver would be as easy as replacing selenium2 with webdriver-classic in your .behat.yml.

@uuf6429
Copy link
Member

uuf6429 commented Feb 26, 2024

  1. we're using a non-existing URL in the example;
  2. the actions we're doing on the page also are non-real;

The examples would need to be adapted for that. E.g. we could use example.com urls, with a flow that really works.

  1. executing example code from within a test suite isn't the same as the real user is executing it (standalone PHP file), where no classes are auto-loaded yet.

Agreed, it's not the same, but if we - for example - remove/deprecate/change the click() method, we would not forget to update this example, since the test would fail.

The purpose here is that the code - from our perspective - should work. If the user misses/breaks something, that's their responsibility.

@uuf6429 uuf6429 merged commit 3b449c5 into minkphp:main Feb 26, 2024
29 checks passed
@aik099 aik099 deleted the readme-improvements branch February 26, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants