search

minkphp / driver-testsuite

Functional testsuite for Mink drivers
MIT License
8 stars 29 forks source link

Fixed file upload test during the build on Selenium 3 #88

Closed aik099 closed 8 months ago

aik099 commented 8 months ago

Fixes this error from the https://github.com/minkphp/MinkSelenium2Driver/pull/383:

2) Behat\Mink\Tests\Driver\Js\ChangeEventTest::testSetValueChangeEvent with data set "file" ('the-file', '/home/runner/work/MinkSeleniu...e1.txt', '/home/runner/work/MinkSeleniu...e2.txt')
WebDriver\Exception\InvalidArgument: File not found: /home/runner/work/MinkSelenium2Driver/MinkSelenium2Driver/vendor/mink/driver-testsuite/web-fixtures/file1.txt
Build info: version: '3.141.59', revision: 'e82be7d358', time: '2018-11-14T08:25:53'
System info: host: 'fv-az697-197', ip: '10.1.0.44', os.name: 'Linux', os.arch: 'amd64', os.version: '5.15.0-1056-azure', java.version: '1.8.0_292'
Driver info: driver.version: unknown

Inspired by the @mvorisek work on the https://github.com/minkphp/driver-testsuite/pull/60.

mvorisek commented 8 months ago

LGTM

uuf6429 commented 8 months ago

I don't get why this change is needed, to be honest. My first guess is that Selenium2Driver does not "share" the fixtures with the browser container - here's how it's done in the classic driver, which works hand-in-hand with the config here. (Side note: why was it working before for Selenium2?)

Additionally, now that I think about it, we should also have a test checking that the correct form (file) data has been POSTed (apparently this test is only checking change events?).

_PS: at the very least, I would avoid having vendor/github-specific code in some random test class. It would make more sense to define an ISCI / isCi() variable/config method.

aik099 commented 8 months ago

I don't get why this change is needed, to be honest. My first guess is that Selenium2Driver does not "share" the fixtures with the browser container - here's how it's done in the classic driver, which works hand-in-hand with the config here.

@uuf6429 , My second thought today, after submitting this PR yesterday was how file upload tests were passing for you with no changes on the WebDriver code side. Looking through the build config of the https://github.com/minkphp/webdriver-classic-driver I've also stumbled upon the mapping and created another PR (see https://github.com/minkphp/MinkSelenium2Driver/pull/385), where I'm adding mapping + overriding the mapRemoteFilePath as you did.

(Side note: why was it working before for Selenium2?)

@uuf6429 , it was working, as @mvorisek had discovered, because Selenium 2 either wasn't checking that the to-be-uploaded file exists on disk OR was allowing relative file paths (with ../../ as we've used to have). I don't recall which of 2 was actually happening or both.

Additionally, now that I think about it, we should also have a test checking that the correct form (file) data has been POSTed (apparently this test is only checking change events?).

Agreed. Created https://github.com/minkphp/driver-testsuite/issues/89 . If you'd like you can create a PR.

_PS: at the very least, I would avoid having vendor/github-specific code in some random test class. It would make more sense to define an ISCI / isCi() variable/config method.

Agreed.