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

Puppeteer keeping the tab open #276

Closed
wants to merge 28 commits into from
Closed

Conversation

hors04
Copy link

@hors04 hors04 commented Aug 23, 2019

This new feature gives the option to run tests in the same tab and browser without closing either hence avoiding any timeouts.

Summary

The point of this added feature is to have an option which lets multiple tests run in the same tab. We have about 19 tests which would run in a separate tab one after another(--runInBand), but because of the different tabs ( perhaps something to do with the cache of the browser) the information will get lost in the process and the tests would timeout. This would happen for a random number of tests.We also use docker to run all the tests, not just by terminal.

Test plan

I ran 15 tests for this example because the other 4 can't be used at the moment. A colleague is working on a bug which involves elements used in those 4 tests. I ran the "jest --runInBand , , ..." command in VS Code terminal to demonstrate the output.

This is the result using the same tab where 4/5 tries were successful and one try failing due to the timeout:

test failed

This is the result using the same tab where 5/5 tries were successful:

tests passed

Note 1: The higher the number of tests , the higher the chance of multiple tests failing due to timeout.
Note 2: When the tests are run by docker , the chance of multiple tests failing is even higher.

This new feature gives the option to run tests in the same tab and browser without closing either hence avoiding any timeouts.
@gregberge
Copy link
Member

Hello @hors04, tests are not passing, could you fix it?

@hors04
Copy link
Author

hors04 commented Aug 28, 2019

Hello @hors04, tests are not passing, could you fix it?

Hello , I've fixed the errors but there seems to be this one new error that says it can't read the property of a variable that I've already added to "jest-puppeteer.config.js" , file which is "keepTabOpen" . It works on my puppeteer package that I use with my tests though. I don't know at the moment what the problem is so it may take a while since I've got other work to do.

@gregberge
Copy link
Member

OK @hors04, anyway we can't merge it if tests are not passing. So we have to wait.

@hors04
Copy link
Author

hors04 commented Sep 10, 2019

@neoziro Hello again , so I finally fixed the code. The feature of keeping the tab open works only for the default mode.

@gregberge
Copy link
Member

@hors04 thanks, I will try to work on it soon. I have to change things before merging it.

@Kradirhamik
Copy link

Hi @neoziro, when will this be merged please?

@Lorenzovds
Copy link

Any update on this? Would cleanup my test flow a lot 🙂

Copy link
Member

@gregberge gregberge left a comment

Choose a reason for hiding this comment

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

The code looks a bit complicated. I don't want to merge something not stable.

await this.global.jestPuppeteer.resetPage()
} else
if (config.browserContext === 'default' || !config.browserContext) {
this.global.context = await this.global.browser.browserContexts()[0];
Copy link
Member

Choose a reason for hiding this comment

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

Why await?

Copy link
Author

Choose a reason for hiding this comment

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

Await is unnecessary. I may have kept it initially for testing purposes.

} else
if (config.browserContext === 'default' || !config.browserContext) {
this.global.context = await this.global.browser.browserContexts()[0];
const [pageOne,] = await this.global.browser.pages();
Copy link
Member

Choose a reason for hiding this comment

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

Why "," ?

Copy link
Author

Choose a reason for hiding this comment

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

browser.pages() returns an array of pages, so we'll always need the first page/element/tab/ default tab that is required to open chrome, the one where the test is run within.

Copy link
Member

Choose a reason for hiding this comment

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

So why adding a "," if you only need the first one?

@gregberge
Copy link
Member

@hors04 can you please fix the code?

@Kradirhamik
Copy link

Any news on this please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants