search

Codeception / lib-innerbrowser

InnerBrowser
MIT License
79 stars 19 forks source link

Added missing navigation methods #35

Closed TavoNiievez closed 2 years ago

TavoNiievez commented 3 years ago

Still a WIP.

Added methods:

To do:

Fixes #33

TavoNiievez commented 3 years ago

@ThomasLandauer Please give us your opinion regarding the names of these new methods.

ThomasLandauer commented 3 years ago

Sure :-)

restartBrowser

What is this needed for?

It flushes history and all cookies.

The history is probably the unimportant part (cause as long as I don't moveBack, I don't care about it, right?). So the main use case is to delete the cookies?
I'm asking, cause "restart" is usually what you do after something crashed ;-)

reloadPage

What does it do exactly?

$I->amOnPage(...);
$I->submitForm(...);
$I->reloadPage(); // reload the page from line 1, or resubmit the form from line 2?

So assuming that it resubmits the form (at least this is what I had in mind at https://github.com/Codeception/lib-innerbrowser/issues/33), I'd rather call it something like resendRequest() or even resendLatestRequest(). How is reloadPage in Acceptance tests https://codeception.com/docs/modules/WebDriver#reloadPage acting? Cause in the end, they both should obviously be called the same and do the same ;-)

Naktibalda commented 3 years ago

Answers to @ThomasLandauer questions:

What does it do exactly?

reloadPage and restartBrowser are very simple wrappers for BrowserKit methods.

WebDriver reloadPage method is a very simple wrapper for a single browser function too: https://github.com/Codeception/module-webdriver/blob/1.2.0/src/Codeception/Module/WebDriver.php#L2089-L2093

ThomasLandauer commented 3 years ago

reloadPage

I think resend(Latest)Request is better here! "send" is also used in REST module, e.g. https://codeception.com/docs/modules/REST#sendPost The probable use case is resending some (complicated) request, and load just doesn't fit in this case. If somebody really wants to reload the page, they can easily repeat amOnPage() - since its argument (URL) can't be very complicated ;-) About WebDriver: When pressing F5 after a POST, the browser raises a dialog to confirm resubmission. Confirming this question is not included in WebDriver's $this->webDriver->navigate()->refresh();, right?

TavoNiievez commented 3 years ago

@ThomasLandauer I have no objection to the name being 'resendLatestRequest', I prefer that you decide those things. I just want to know if you are totally sure you prefer that one, as 'reloadPage' is something anyone without technical knowledge can infer what it does, while 'resendLatestRequest' in my opinion at least you have to know that it is a request in the HTTP context and the flow of information.

ThomasLandauer commented 3 years ago

There are only two hard things in Computer Science: cache invalidation and naming things.

https://martinfowler.com/bliki/TwoHardThings.html

Short answer: A non-programmer won't understand this anyway ;-)

Long answer:

Again: The use case I'm seeing is repeating POST, PUT, DELETE etc. - not just "reloading" (GET) the page, since in a testing scenario this would be useless (what could have changed?). If that's a wrong assumption, tell me!

  1. A non-programmer will never think that repeating the same request might lead to trouble, and is something we need to test. Therefore they will never write this method.
  2. If a non-programmer reads "reloadPage", I bet they'll think it's going to reload the page containing the form; not repeating the form-submission. So using a term they don't quite understand, is actually a plus - since it's better to make them stop and think/ask, instead of continue with a wrong understanding.
  3. If a programmer reads "reloadPage", they might (a) understand it right, (b) understand it wrong, or (c) don't know what is going to be reloaded.

So I stick to my opinion ;-) But we could use "repeat" instead of "resend" - maybe that sounds a little nicer? So I'm suggesting 4 options which are all OK for me :-)

TavoNiievez commented 3 years ago

So I stick to my opinion ;-)

That's enough for me. I think 'resend' makes more sense here. I'll change it to 'resendLatestRequest'.

There are only two hard things in Computer Science: cache invalidation and naming things.

Yes, in that sense I leave the hard part to you haha :-) Thank you.

Naktibalda commented 3 years ago

@TavoNiievez are you going to rename it?

Better explanation of behaviour in the docblock would help to avoid some of confusion that you discussed above.

TavoNiievez commented 2 years ago

I couldn't get the tests for this new feature to pass at the time. I'll work on this later after the release of Codeception 5. It's not a priority at the moment.