Fix for #14 #15

pdebruic · 2018-07-12T19:23:07Z

No description provided.

…hanged structure

…hort so strip those out before parsing

Rinzwind · 2018-09-09T14:10:38Z

Sorry for taking so long to take a look at this! I see there's a comment that may still need some work? I'm referring to the comment in BPRemoteWebDriver>>#initWithCapabilities: that says "I don't know how to […]":

b0bc459#diff-f252e53767a232a2a9f0a3b7815172a6R15

I'm not sure how to reproduce the situation where the 302-response is given. I tried using selenium-server-standalone-3.13.0.jar and chromedriver 2.40.565386 but I didn't hit the bug. The code to handle responses with status code 303 and 302 was originally introduced in Parasol-Core-KrisGybels.25 and Parasol-Core-KrisGybels.26, but the comments unfortunately don't mention why this was added and under what conditions those responses are given. The bug with the missing return statement seems to have been introduced in the changes between Parasol-Core-chaerle.42 and Parasol-Core-JohanBrichau.39 (there are some versions missing there from the SqueakSource3 repository).

pdebruic · 2018-09-10T16:23:11Z

For that comment, IIRC, when handling the 302 response the browser capabilities arent set. The 302 redirect from the selenium server doesn't have the response body, so the capabilities cannot be set. I think its safe to ignore for now until its a problem for someone in the future. Everything seems to work fine without setting them.

…

On Sep 9, 2018, at 7:10 AM, Kris ***@***.***> wrote: Sorry for taking so long to take a look at this! I see there's a comment that may still need some work? I'm referring to the comment in BPRemoteWebDriver>>#initWithCapabilities: that says "I don't know how to […]": b0bc459#diff-f252e53767a232a2a9f0a3b7815172a6R15 I'm not sure how to reproduce the situation where the 302-response is given. I tried using selenium-server-standalone-3.13.0.jar and chromedriver 2.40.565386 but I didn't hit the bug. The code to handle responses with status code 303 and 302 was originally introduced in Parasol-Core-KrisGybels.25 and Parasol-Core-KrisGybels.26, but the comments unfortunately don't mention why this was added and under what conditions those responses are given. The bug with the missing return statement seems to have been introduced in the changes between Parasol-Core-chaerle.42 and Parasol-Core-JohanBrichau.39 (there are some versions missing there from the SqueakSource3 repository). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

Rinzwind · 2018-09-11T09:28:00Z

It would be better if we could fix that now and have a test for it. I don't know (or remember) how to reproduce the situation where one gets the 302 response. Do you have some info on how to reproduce that?

pdebruic · 2018-09-11T11:46:44Z

I disagree but also don’t understand/remember what the capabilities inst var is used for. You could just take out the #ifTrue: line if it’s aesthetically displeasing. I mean if I had left the #ifTrue: line (& comment) out would we even be talking about this? Bad 302 handling hasn’t been a problem for a while based on your analysis of the change history. I think you can configure zinc to give you a 302 for every request to a url right if you append something like a UUID as the last path element eg ‘Http://localhost/always-redirect/‘ , UUID new asString And that would prevent the browser caching the redirect permanently. I’ll try to remember why the 302 was occurring but I think it was redirecting from http to https But the headers that have the capabilities don’t come through in the 302 so either selenium needs to be compelled to resend those headers or I don’t know what

…

On Sep 11, 2018, at 2:28 AM, Kris ***@***.***> wrote: It would be better if we could fix that now and have a test for it. I don't know (or remember) how to reproduce the situation where one gets the 302 response. Do you have some info on how to reproduce that? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

Rinzwind · 2018-09-17T07:42:02Z

The code for handling responses with status code 302 or 303 to the "POST /session" requests was originally added for compatibility with older versions of selenium server. That is, Parasol-Core-KrisGybels.24 adapted Parasol to work with v2.35.0, while this was modified in Parasol-Core-KrisGybels.25 to still support older versions as well. I'd like to better understand why a recent version of selenium server like v3.13.0 would still require this old compatibility code. Could you tell us how to reproduce such a response and/or give a dump of one?

As for the capabilities: if the response to the "POST /session" request does not include them, I'm not sure how to get them. Previously, the JSON wire protocol specified that the server should support GET requests on "/session/{sessionID}" with the response including the session's capabilities. But this was dropped from the newer W3C webdriver protocol (see issue #12). I'm not sure the newer protocol specifies a way of getting the capabilities other than from the response to the "POST /session" request.

pdebruic · 2018-12-11T00:08:12Z

Here is a dump

BPRemoteWebDriver>>sessionIdFromResponse:
BPRemoteWebDriver>>initWithCapabilities:
BPRemoteWebDriver class>>withCapabilities:
BPWebDriverManagerResource>>driverWithCapabilities:on:
StafferParasolCalendarMovementTests(BPWebDriverTestCase)>>setUp
StafferParasolCalendarMovementTests(StafferParasolTests)>>setUp
[ self setUp.
self performTest ] in StafferParasolCalendarMovementTests(TestCase)>>runCase in Block: [ self setUp....
BlockClosure>>ensure:
StafferParasolCalendarMovementTests(TestCase)>>runCase
StafferParasolCalendarMovementTests(BPWebDriverTestCase)>>runCase
[ aTestCase runCase ] in [ [ aTestCase runCase ]
	on: Halt
	do: [ :halt | 
		"if test was halted we should resume all background failures 
			to debug all of them together with test process"
		failedProcesses keysDo: #resume.
		halt pass ] ] in TestExecutionEnvironment>>runTestCaseSafelly: in Block: [ aTestCase runCase ]
BlockClosure>>on:do:
[ [ aTestCase runCase ]
	on: Halt
	do: [ :halt | 
		"if test was halted we should resume all background failures 
			to debug all of them together with test process"
		failedProcesses keysDo: #resume.
		halt pass ] ] in TestExecutionEnvironment>>runTestCaseSafelly: in Block: [ [ aTestCase runCase ]...
BlockClosure>>on:do:
TestExecutionEnvironment>>runTestCaseSafelly:
[ self runTestCaseSafelly: aTestCase ] in [ [ self runTestCaseSafelly: aTestCase ]
	ensure: [ testCompleted := true.
		watchDogSemaphore signal ].	"signal that test case completes"
self checkForkedProcesses ] in TestExecutionEnvironment>>runTestCase: in Block: [ self runTestCaseSafelly: aTestCase ]
BlockClosure>>ensure:
[ [ self runTestCaseSafelly: aTestCase ]
	ensure: [ testCompleted := true.
		watchDogSemaphore signal ].	"signal that test case completes"
self checkForkedProcesses ] in TestExecutionEnvironment>>runTestCase: in Block: [ [ self runTestCaseSafelly: aTestCase ]...
BlockClosure>>ifCurtailed:
TestExecutionEnvironment>>runTestCase:
[ testEnv runTestCase: aTestCase ] in DefaultExecutionEnvironment>>runTestCase: in Block: [ testEnv runTestCase: aTestCase ]
[ self value: anExecutionEnvironment.
anExecutionEnvironment activated.
aBlock value ] in CurrentExecutionEnvironment class>>activate:for: in Block: [ self value: anExecutionEnvironment....
BlockClosure>>ensure:
CurrentExecutionEnvironment class>>activate:for:
TestExecutionEnvironment(ExecutionEnvironment)>>beActiveDuring:
DefaultExecutionEnvironment>>runTestCase:
CurrentExecutionEnvironment class>>runTestCase:
StafferParasolCalendarMovementTests(TestCase)>>runCaseManaged
[ aTestCase announce: TestCaseStarted withResult: self.
aTestCase runCaseManaged.
aTestCase announce: TestCaseEnded withResult: self.
self addPass: aTestCase ] in TestResult>>runCaseForDebug: in Block: [ aTestCase announce: TestCaseStarted withResult: ...etc...

To get it I installed parasol in a pharo 6.1 image on mac, started the selenium jar file with

java -jar selenium-server-standalone-3.141.59.jar

then attempted to run a test without changing anything.

Why do the capabilities matter?

Rinzwind · 2018-12-11T18:24:50Z

I have not yet been able to reproduce the issue. I have tried using selenium-server-standalone-3.141.59.jar and chromedriver v2.45. I will try again later, but I'm currently hitting an issue with loading Parasol (related to the package “Zinc-Seaside”, I don't know yet what the problem there is).

@pdebruic: if you wouldn't mind, could you provide some additional debugging output?

Start selenium server with options to enable logging to two files:

java -Dwebdriver.chrome.logfile=chromedriver.log -Dwebdriver.chrome.args=--verbose -jar selenium-server-standalone-3.141.59.jar -log selenium.log

Extend BPPharoPlatform>>#handleRequest: to log the response to the Transcript:

handleRequest: aBlock

	| response contents|
	response := aBlock value.
	response writeToTranscript.
	"(^ Add the above line, leave rest of the method unchanged)"

Could you provide the two log files and the Transcript output for running the following (which as far as I understand is what triggers the issue)?

BPRemoteWebDriver withCapabilities: BPDesiredCapabilities chrome.

Rinzwind · 2018-12-11T18:38:25Z

For reference, the documentation about “POST /session” can be found at the following two pages, for the older JSON wire protocol and the newer W3C protocol respectively:

I cannot tell how a response with status code 302 or 303 (and no body) would still be in accordance with these specifications (as I mentioned above, older versions of selenium/webdriver did respond using these status codes, but I'm not sure why newer versions would still exhibit that behavior).

…emoteWebDriver>>#sessionIdFromResponse: as this code was originally added for compatibility with pre-v2.35.0 versions of selenium server, we no longer support those versions and the code has also been broken for quite a while (for more details see: #15 (comment)).

Rinzwind · 2019-06-27T07:11:34Z

Closing this pull request, see: #14 (comment).

pdebruic added 2 commits July 12, 2018 12:19

fix 301/302 request session id bug. update README as wikipedia page c…

b0bc459

…hanged structure

Selenium JSON responses seem to be padded with null characters when s…

83ad7f2

…hort so strip those out before parsing

pdebruic mentioned this pull request Jul 12, 2018

Unexpected response status code 302 while creating web driver session. #14

Closed

jbrichau requested a review from Rinzwind July 18, 2018 16:11

Rinzwind closed this Jun 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #14 #15

Fix for #14 #15

pdebruic commented Jul 12, 2018

Rinzwind commented Sep 9, 2018

pdebruic commented Sep 10, 2018 via email

Rinzwind commented Sep 11, 2018

pdebruic commented Sep 11, 2018 via email

Rinzwind commented Sep 17, 2018

pdebruic commented Dec 11, 2018 •

edited

Loading

Rinzwind commented Dec 11, 2018

Rinzwind commented Dec 11, 2018

Rinzwind commented Jun 27, 2019

Fix for #14 #15

Fix for #14 #15

Conversation

pdebruic commented Jul 12, 2018

Rinzwind commented Sep 9, 2018

pdebruic commented Sep 10, 2018 via email

Rinzwind commented Sep 11, 2018

pdebruic commented Sep 11, 2018 via email

Rinzwind commented Sep 17, 2018

pdebruic commented Dec 11, 2018 • edited Loading

Rinzwind commented Dec 11, 2018

Rinzwind commented Dec 11, 2018

Rinzwind commented Jun 27, 2019

pdebruic commented Dec 11, 2018 •

edited

Loading