WAR-1141: selenium:not able to launch the browser with url if it is not the part of world wide web by brettleehari · Pull Request #66 · warriorframework/warriorframework

brettleehari · 2017-07-11T16:48:27Z

Current behaviour : For url that are not part of "WWW" as computed by urllib2 package of python, we stop navigating to that particular url an mark the step as failed.
Change : urllib2 result has no impact of the step and on whether we should navigate to that particular url. Instead we warn the user based on urllib2 responses.

…to the url. Now we throw warning/ error to the user

Annam16

approving this pull request by testing it with the testcase which is available in warrior_fnc_tests/warrior_tests/testcases/selenium_func_tests/generic_tests/tc_browser_launch_url_check.xml. But this test case requires some changes related to this fix. Please raise a separate pull request for this in bitbucket.

…to master * commit '1e524a15a2d6bff69bb16004dc60959066a91e11': Added the .travis.yml file

sanikakulkarni

Hi @brettleehari,

Please add a testcase covering this scenario in the selenium regression tests so that this feature is tested each time changes are made in the Selenium module. If you have already created regression tests for this change, please provide a link in the JIRA ticket

Since you require a non-www based url, you can create a "dummy" html file and store it in the Config_files directory and open that file from the browser_launch (another scenario would be to use the navigate to url) keyword (I think a relative path should be fine). Once the browser_launch keyword is successful, you can verify if the file really opened by trying to "locate" a tag in that file using any of our "locate_element" keywords.

Let me know if you can think up of anything else.

sanikakulkarni

Hi @brettleehari,

Please run the selenium regression tests for this change and upload the zip into the ticket. Please look into WAR-1303 before running the selenium regression project.

sanikakulkarni

Hi @Annam16,

The testcase that you mention in the comments does not have a non-www based url - or does it and I just missed it, please let me know.

You also mention some changes related to this pull request, can you further elaborate on what those are?

@brettleehari, has a ticket been created to address those changes that Annalakshmi has mentioned, please let me know.

Annam16 · 2017-07-19T06:08:15Z

Hi @sanikakulkarni .
I have tested this fix with the test case which is available in warrior_fnc_tests(tc_browser_launch_url_check.xml) and it is failing . it requires changes like changing the context of some test step from negative to positive because earlier we will not be launching the url if it is not a valid url. Attached the log in JIRA . And also it contains non-www based url to test this fix.

sanikakulkarni · 2017-07-31T18:49:22Z

-            status = False
-        if status == False:
-            print_error("Incorrect URL provided")
-        return status, url


Hi @brettleehari,
Changing the number of parameters being returned is not advisable as it breaks backward compatibility for anyone making use of this function outside of the WSelenium keywords.

Agreed and changed.

sanikakulkarni · 2017-07-31T18:50:41Z

-                        if result == True:
-                            result = self.browser_object.go_to(url,
-                                                               browser_inst)
+                        self.browser_object.check_url(browser_details["url"])


Hi @brettleehari,
There is not point calling the function check_url() in line 185 as it always is returning None. Even if an Exception occurs in check_url(), None is being returned - hence there is no indication that the url navigation did not finish successfully (apart for the error message being displayed on the console) and None is being returned even when the url being checked is correct.

Please let me know what is needed out of the check_url() function so that I understand the code change better.

The bug is url navigation has nothing to do check_url(). So check_url should not impact warrior attempting to navigate to the specified url.
Check url just checks the function and no navigation is performed in this function.
Check_url is based on urllib2 module, which can validate public urls with http/https as schemes.

The current purpose of check_url is to send warning/info/error to the user with appropriate message to the user. This information is captured in the description part of the pull request.

sanikakulkarni · 2017-07-31T19:05:25Z

Reply to:

Hi @sanikakulkarni .
I have tested this fix with the test case which is available in warrior_fnc_tests(tc_browser_launch_url_check.xml) and it is failing . it requires some changes.Attached the log in JIRA.

Hi @Annam16,

Please let me know why this pull request was approved if tc_browser_launch_url_check.xml is failing. Please let me know if tickets have been created to address any issue found through/by the code changes made to satisfy WAR-1141

sanikakulkarni

Hi @brettleehari,

please create tests to cater to the functionality being added via this pull request (tc containing a url without the www) and create a pull request to the internal selenium functionality tests.

Please add a link to that pull request into the ticket WAR-1141

…framework into bugfix/WAR-1141

Annam16 · 2017-08-01T13:25:52Z

Hi @sanikakulkarni
I have approved this pull request even though "tc_browser_launch_url_check.xml" failed because, it has to be a separate pull request in bitbucket in warrior_fnc_tests. I have added this as a comment and approved it.

No new tickets has been raised related to war-1141 fix. To make "tc_browser_launch_url_check.xml" pass/valid I have added this tc in WAR-1303 itself.

And this test case alone is enough to test this fix, since it contains non-www based url.

brettleehari · 2017-08-02T18:04:42Z

@sanikakulkarni
I have added functional tests for the fix with the same ticket and i did not create a new ticket for editing a functional test.
Moreover since we are reverting back a feature, all we have to do it flip the status of the functional tests which were created for the earlier feature. Hope this clarifies everything. Kindly review and let me know if you have any more comments.

This comment has been addressed in the next commit

sanikakulkarni · 2017-08-03T17:21:50Z

-                            result = self.browser_object.go_to(url,
-                                                               browser_inst)
+                    url = browser_details["url"]
+                    if url is not None:


Hi @brettleehari,

The check_url() function has a few problems in it. on line 188, it is opening the URL to get the associated status code. This can be problematic as the url being opened can be (for example) performing a post request. Now, on line 188, that request is performed, and hence when we actually try to open it via selenium, it may cause errors. It would be better if the check_url() is not used at all in our keywords - the function can be kept as is to maintain backward compatibility - we just won't use it. It would be very helpful is a disclaimer in the function's docstrings is added telling the user exactly what is happening inside that function. I would recommend that all changes made to the check_url() function on this pull request be undone.

Now, I want to list down use cases of what should fail and what should not - I am working with what I see when I type into a browser's address bar:

Column 2 and 3 represent what we want out of our go_to() function. Column 4 notes what selenium** does with those URLs. Column 5 represents whether selenium satisfies our requirements or not.

**Please note that I messed around with my installed selenium package to get the internal status codes. Selenium DOES NOT provide a way to access status codes via its public methods.

To this, I can only recommend removing the usage of the check_url() from our browser_launch keyword, catching the exceptions that may occur in the internal go_to() and returning a status accordingly. Every use-case that selenium does not satisfy should be documented in the docstrings for browser_launch() and navigate_to_url() keywords to ensure that this knowledge is made available to users.

I would also recommend creating a ticket to handle the unsatisfied use-cases so that they can be worked upon at a later time either when needed or when selenium starts supporting the functionality.

Please also make sure that the regression testcase that you have updated contains all the use-cases above with corresponding <context> tags.

Feel free to add more use-cases and let me know if you need internal selenium information for those use-cases.

Let me know if you have any suggestions/questions.

@brettleehari what is the status of the above comments?

@bjrr We had a call and explained to Sanika that we cannot do the operation as mentioned in sheet with examples.
Addressed all the concerns.

…nch keyword status does not depend on succussful navigation to the url

sanikakulkarni · 2017-09-27T22:40:02Z

Hi @brettleehari,

I am posting an email from a conversation that happened between us about this ticket:

The check_url function needs to be removed from usage as it “opens” the url to retrieve the status. This implementation is faulty and can result in failed testcases (in cases where the URL should not be opened more than once). The check url function can exist in our utils (backward compatibility), but a disclaimer should be added in the check_url() docstrings that explains clearly what this functions does.
As far as navigating to a url (that is incorrect and would result in subsequent step failures), the browser_launch and the navigate_to_url keyword should not ‘care’ about the navigation results and instead a brand new keyword (navigate_to_url_with_verification) should be added that lets the user give a url to it and then also provide a way to test if the url navigation went through (by giving a tag name or page title etc) to verify the page with. Now this keyword would then be able to navigate to the given url and then verify if the navigation was successful.
Proper documentation should be added into the docstrings for browser_launch and navigate_to_url keywords to let the user know that these keywords would only navigate to urls and not verify if the navigation was successful and that if they want such verification, then they should use the navigate_to_url_with_verification keyword

This is to maintain consistency between review comments - and to keep track of the resolution reached about this PR's design.

Sanika

sanikakulkarni · 2017-09-27T22:46:55Z

+        """To check whether the user provided url is valid or not.
+           Returns:
+                    1. status(bool)= True / False.(Whether the url can be reached)
+                    2. url : The actual url itself


Hi @brettleehari,

Please add documentation here that clearly states states that this function would internally "open" a URL to validate it.

Thanks,
Sanika

Done! Explicitly added a note

sanikakulkarni · 2017-09-27T22:48:42Z

Hi @brettleehari,

According to point (2), we should be adding a keyword - navigate_to_url_with_verification - to let user perform verification after going to a URL.

Has a new ticket been created for this?

Further Clarification
If there is a new ticket created for this, please work on it in this pull request as technically, it is a part of this ticket. Please include this in this PR.
If a new ticket has not been created, then this pull request is missing out on point (2) entirely. Please address point (2).

Thanks.

To @sanikakulkarni , @bjrr,
Please check SUPWAR-173 & SUPWAR-104 these are bugs raised by the user community. The bug is the current check of validating the url is invalid and gives a false pass. This bug is addressed by the fix. The point 2 is a enhancement that we discussed which could be useful to the user who wants to verify whether they have landed on the right page. Hence point 2 is an enhancement and is out of scope of the current fix for which the user has raised a bug.
As per @sanikakulkarni e-mail ("Point (2) requires information collection on as to how the user would be able to verify if the navigation was successful and then our already available utils can be used or if utils are not available, they can be added to our selenium utils package and then used.")
Kindly request you to raise a ticket for Enhancement with appropriate direction.

sanikakulkarni · 2017-09-27T22:51:13Z

Hi @brettleehari,

Since it has been decided that browser_launch and navigate_to_url would not "care" about the validity of URL, we need clear documentation in those kws' docstrings which explicitly mention this.

This documentation has only been added for browser_launch. Can this also be added to navigate_to_url?

Thanks.

Hi @sanikakulkarni,
I explicitly added comment "Does not verify url" to keyword "browser_launch" keyword because earlier we supported url verification. Navigate to url keyword "navigate_to_url" never had a verification url inbuilt into it hence I didnt add any comment unnecessarily as keyword "navigate_to_url" does not have any changes due to the current fix.

sanikakulkarni · 2017-09-27T22:52:16Z

        """
-        This will launch a browser.
+        The Keyword would launch a browser and Navigate to the url, if provided by the user.
+        The keyword does not validate the url provided by the user.


Hi @brettleehari,

Can line 53 be emphasized? Like it can be made all caps or have an underline?

Thanks.

@sanikakulkarni I have underlined as per your comment. Is this Warrior convention ? @bjrr What is your view ?

sanikakulkarni · 2017-09-27T22:55:31Z

+                        if urlschema.scheme:
+                            result = self.browser_object.go_to(url, browser_inst)
+                        else:
+                            status = False


Hi @brettleehari,

Instead of setting the status to False in line 197, the result should be set to False - eventually in line 206 the status will become False anyway because we are and-ing result and status there.

result refers to a single iteration the for loop (each browser opened) while status is the status of the entire kw.

Thanks.

sanikakulkarni · 2017-10-03T17:53:58Z

Selenium regression attached in JIRA ticket. Travis passing. Comments addressed. PR approved.

PR ready to be merged.

reduce the severity for urllib2 errors, currently we stop navigating …

2bc0fe2

…to the url. Now we throw warning/ error to the user

brettleehari changed the base branch from master to develop July 11, 2017 16:49

brettleehari requested review from Annam16, bjrr, sanikakulkarni and satyakrish July 11, 2017 16:52

Annam16 previously approved these changes Jul 13, 2017

View reviewed changes

navvrnd pushed a commit that referenced this pull request Jul 14, 2017

Merge pull request #66 in WAR/warriorframework from feature/WAR-1026 …

aceb12b

…to master * commit '1e524a15a2d6bff69bb16004dc60959066a91e11': Added the .travis.yml file

sanikakulkarni reviewed Jul 18, 2017

View reviewed changes

sanikakulkarni reviewed Jul 31, 2017

View reviewed changes

sanikakulkarni suggested changes Jul 31, 2017

View reviewed changes

sudharshan, hariprasad added 2 commits August 1, 2017 04:42

incorporate review comments

f22e48d

Merge branch 'develop' of https://github.com/warriorframework/warrior…

35339cf

…framework into bugfix/WAR-1141

brettleehari dismissed Annam16’s stale review via 35339cf August 1, 2017 14:59

sudharshan, hariprasad added 3 commits August 2, 2017 22:14

change the decision to navigate to an url

b5230b3

improve pylint avoid travis failure

c6853bf

improve pylint avoid travis failure add doc string

7dc22ae

sanikakulkarni reviewed Aug 3, 2017

View reviewed changes

brettleehari added the bug label Aug 11, 2017

sudharshan, hariprasad added 2 commits August 16, 2017 00:55

All usecases in the review is satisfied

3ad2dcb

It was decided to remove check url from browser launch and brwser lau…

b3c3ef9

…nch keyword status does not depend on succussful navigation to the url

brettleehari mentioned this pull request Sep 22, 2017

WAR-724 - To test if all the available selenium functionality in Warrior is compatible with Selenium 3.0 #62

Merged

To explicitly state check_url internally opens the url given

a5cf558

sanikakulkarni reviewed Sep 27, 2017

View reviewed changes

sudharshan, hariprasad and others added 4 commits September 28, 2017 06:04

Underline the text that says the keyword does not validate the url

941e960

changed status to result to accomodate the loop logic

c518daf

Added navigate_to_url_with_verification kw

5d54a9b

Documentation update

8e93c65

sanikakulkarni changed the title ~~1141: selenium:not able to launch the browser with url if it is not the part of world wide web~~ WAR-1141: selenium:not able to launch the browser with url if it is not the part of world wide web Oct 2, 2017

bjrr removed their request for review October 2, 2017 23:14

satyakrish approved these changes Oct 3, 2017

View reviewed changes

sanikakulkarni approved these changes Oct 3, 2017

View reviewed changes

satyakrish merged commit 07387f4 into develop Oct 4, 2017

Uh oh!

Conversation

brettleehari commented Jul 11, 2017

Uh oh!

Annam16 left a comment

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni left a comment

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annam16 commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanikakulkarni Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettleehari Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettleehari Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanikakulkarni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Annam16 commented Aug 1, 2017

Uh oh!

brettleehari commented Aug 2, 2017 • edited by sanikakulkarni Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanikakulkarni Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjrr Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

brettleehari Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni commented Sep 27, 2017

Uh oh!

sanikakulkarni Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

brettleehari Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni commented Sep 27, 2017 • edited by brettleehari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanikakulkarni commented Sep 27, 2017 • edited by brettleehari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanikakulkarni Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

brettleehari Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

sanikakulkarni Sep 27, 2017

sanikakulkarni left a comment •

edited

Loading

sanikakulkarni left a comment •

edited

Loading

Annam16 commented Jul 19, 2017 •

edited

Loading

sanikakulkarni Jul 31, 2017 •

edited

Loading

sanikakulkarni Jul 31, 2017 •

edited

Loading

brettleehari Aug 2, 2017 •

edited

Loading

sanikakulkarni commented Jul 31, 2017 •

edited

Loading

sanikakulkarni left a comment •

edited

Loading

brettleehari commented Aug 2, 2017 •

edited by sanikakulkarni

Loading

sanikakulkarni Aug 3, 2017 •

edited

Loading

brettleehari Sep 20, 2017 •

edited

Loading

sanikakulkarni commented Sep 27, 2017 •

edited by brettleehari

Loading

sanikakulkarni commented Sep 27, 2017 •

edited by brettleehari

Loading