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

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

Closed

Conversation

aik099
Copy link
Member

@aik099 aik099 commented Feb 24, 2024

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
Contributor

LGTM

@uuf6429
Copy link
Member

uuf6429 commented Feb 24, 2024

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
Copy link
Member Author

aik099 commented Feb 25, 2024

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