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

Added missing navigation methods #35

Closed

Conversation

TavoNiievez
Copy link
Member

@TavoNiievez TavoNiievez commented Apr 8, 2021

Still a WIP.

Added methods:

  • moveForward
  • reloadPage
  • restartBrowser

To do:

  • Tests ?

Fixes #33

@TavoNiievez
Copy link
Member Author

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

@ThomasLandauer
Copy link
Member

Sure :-)

  • moveForward is the opposite of moveBack?! => perfect name IMO :-)

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

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

  • restartBrowser: I was thinking about calling it something like resetCookies. But since (a) Symfony calls it restart and (b) they might add more functionality (which will then probably be reflected here), I now say: Let's keep restartBrowser. This won't be our "best-selling" function anyway ;-)

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

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

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 :-)

  • repeatRequest
  • repeatLatestRequest
  • resendRequest
  • resendLatestRequest

@TavoNiievez
Copy link
Member Author

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

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

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.

@TavoNiievez TavoNiievez closed this Feb 4, 2022
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.

Repeat latest request in functional test?
3 participants