Skip to content

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

Closed
aik099 wants to merge 1 commit into
minkphp:masterfrom
aik099:file-upload-within-docker-image-fix
Closed

Fixed file upload test during the build on Selenium 3#88
aik099 wants to merge 1 commit into
minkphp:masterfrom
aik099:file-upload-within-docker-image-fix

Conversation

@aik099

@aik099 aik099 commented Feb 24, 2024

Copy link
Copy Markdown
Member

Fixes this error from the minkphp/MinkSelenium2Driver#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 #60.

@aik099 aik099 changed the title Fixed file upload test during the build Fixed file upload test during the build on Selenium 3 Feb 24, 2024
@mvorisek

Copy link
Copy Markdown
Contributor

LGTM

@uuf6429

uuf6429 commented Feb 24, 2024

Copy link
Copy Markdown
Member

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 IS_CI / isCi() variable/config method.

@aik099

aik099 commented Feb 25, 2024

Copy link
Copy Markdown
Member Author

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 minkphp/MinkSelenium2Driver#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 #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 IS_CI / isCi() variable/config method.

Agreed.

@aik099 aik099 closed this Feb 25, 2024
@aik099
aik099 deleted the file-upload-within-docker-image-fix branch February 25, 2024 13:53
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