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

Support for Incognito session #996

Merged
merged 48 commits into from
Feb 25, 2020
Merged

Conversation

saikrishna321
Copy link
Member

@saikrishna321 saikrishna321 commented Jan 5, 2020

@gaugebot gaugebot bot added the cla-signed label Jan 5, 2020
@gaugebot gaugebot bot requested a review from nehashri January 5, 2020 09:56
@saikrishna321 saikrishna321 changed the title WIP - Support for Incognito sessiomn WIP - Support for Incognito session Jan 5, 2020
Copy link
Member

@zabil zabil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work and feature to be working with multiple browsers!

Before doing the technical review here are some API thoughts.

One use case for this feature is supporting different user roles in the same workflow. For e.g. consider a hypothetical admin and non admin system

  • User logs in
  • User requests creates and submits and item
  • Admin logs in
  • Admin approves the item
  • Use checks if the item if approved

A readable api for this workflow would be

openBrowser({profile: ‘user’})
write("user", into(‘Username:’))
write("password", into(‘Password:’))
click(“Sign In”)
click(“Submit Item”)
openBrowser({profile: ‘admin’))
write(‘admin’, into(‘Username:’))
write(‘password’, into(‘Password:’))
click(“Sign In”)
click(‘approve’)
switchTo({profile: ‘user”)
text(“approved”).exists()
closeBrowser({profile: "user"})
closeBrowser({profile: "admin"})

Notice that there is no incognito mode. If the user wants to launch incognito mode it can be

openBrowser({profile: ‘user’, incognito: true})

This way the user can also pass arguments to openBrowser()

It's better not to have a seperate incognito mode api.
Does that make sense?

More details about profiles at #921

@saikrishna321 saikrishna321 changed the title WIP - Support for Incognito session Support for Incognito session Jan 7, 2020
@saikrishna321
Copy link
Member Author

@zabil @negiDharmendra Can you please review now?

Docstring is pending.

@zabil
Copy link
Member

zabil commented Jan 10, 2020

It's better to handle support for profiles in another PR using the profile directory of chrome.
For this feature the recommendation is an API as follows.

openWindow(url, { name: "new window"});

This opens a window by default. If the user wants to open it in incognito mode

openWindow(url, { name: "private", incognito: true})

For switching windows

switchTo("new window")
switchTo("private")

For closing the window

closeWindow("name") // or can be a regex to close multiple windows with the same pattern

A representative use case could look like

openBrowser()
goto("example.com")
write("user", into(‘Username:’))
write("password", into(‘Password:’))
click(“Sign In”)
click(“Submit Item”)

openWindow("example.com", {name: "admin", incognito: true})
write(‘admin’, into(‘Username:’))
write(‘password’, into(‘Password:’))
click(“Sign In”)
click(‘approve’)

switchTo("User: Item lists") // Assuming title of the page
text(“approved”).exists()
closeWindow("admin") // name of the incognito window
closeBrowser() // Also closes all open windows.

@saikrishna321
Copy link
Member Author

@zabil I have fixed all the feedbacks.

@zabil zabil requested review from negiDharmendra and Debashis9012 and removed request for nehashri January 16, 2020 14:13
lib/browserContext.js Outdated Show resolved Hide resolved
lib/browserContext.js Outdated Show resolved Hide resolved
};

if (targetUrl.includes('about:blank')) {
newCritarget();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should await newCritarget

test/unit-tests/openBrowser.test.js Outdated Show resolved Hide resolved
lib/taiko.js Outdated Show resolved Hide resolved
@Debashis9012
Copy link
Contributor

@saikrishna321,

  1. observation of Test Case 5: For duplicate usage of window name { name : "gauge"} currently we are providing the warning message to the user but the problem in this case closeWindow functionality breaks as it will not able to close all the window with the same duplicate window name.

    For this first time closeWindow works successfully:

    closeWindow("gauge")
    ✔ Window with name gauge closed

    afterwards, it's throwing below error:

    > closeWindow("gauge")
    ✘ Error: Cannot read property 'targetId' of undefined, run `.trace` for more info.
    

    We had discussed this in the stand up, all of us agreed on switching to the existing window with
    the provided name and printing the warning message.

  2. Most of the time openWindowis not able to load the url and repl hangs where open window with incognito mode works fine. So we agreed on changing the behaviour of openWindow to open window in incognito mode by default and if the user wants to open normal window then they can pass incognito with false parameter.
    openWindow(url, { name: "private", incognito: false})

@zabil
Copy link
Member

zabil commented Jan 29, 2020

We had discussed this in the stand up, all of us agreed on switching to the existing window with
the provided name and printing the warning message.

For example

> openWindow({name: "unique"})
> click("login")
> openWindow({name: "unique"})
A window with the name "unique" is already open, switching to it.

This is a warning and not an error.

@saikrishna321
Copy link
Member Author

We had discussed this in the stand up, all of us agreed on switching to the existing window with
the provided name and printing the warning message.

For example

> openWindow({name: "unique"})
> click("login")
> openWindow({name: "unique"})
A window with the name "unique" is already open, switching to it.

This is a warning and not an error.

@zabil #996 (comment) we decided previosuly to open a new window with random name. I'm getting confused now.

@zabil
Copy link
Member

zabil commented Jan 30, 2020

we decided previosuly to open a new window with random name. I'm getting confused now.

So sorry for the confusion. I did say that, but I did not consider the effect that it may have on the closeWindow or managing the windows. Hence the new recommendation.

@saikrishna321
Copy link
Member Author

@zabil @Debashis9012 Please review. Fixed all the comments

@zabil zabil merged commit 89b2192 into getgauge:master Feb 25, 2020
@saikrishna321 saikrishna321 deleted the incognito branch March 11, 2020 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants