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

Add end 2 end test sidebar behaviours on mobile and desktop. #6877

Merged
merged 1 commit into from Jun 25, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented May 21, 2018

Description

Sometimes I notice regressions on the sidebar behaviors e.g.:#6876. So I'm adding an end to end test that tests simple sidebar behaviors on mobile and desktop.

To execute the test use:

npm run test-e2e test/e2e/specs/sidebar-behaviour.test.js

The tests will fail because currently we have a bug and #6876 should be merged first.

await setMobileViewPort();
}
export async function setDesktopViewPort() {
await page.setViewport( { width: 1000, height: 700 } );

This comment has been minimized.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 23, 2018

Member

In our case, I think we don't have any difference using emulate. We just test for mobile using the resolution. But besides that, we need after emulating mobile set again the default behavior for the desktop. One option to do that would be using an emulate( DesktopDevice ) but puppeteer only offers mobile devices by default https://github.com/GoogleChrome/puppeteer/blob/master/DeviceDescriptors.js. So we would need to create a desktop mobile device, or change the tests to use a new page in each test case.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 23, 2018

Member

Creating a desktop mobile device is simple it is a JSON object so maybe it is a good idea.

await setMobileViewPort();
// the page was resized to mobile
const width = await page.evaluate(

This comment has been minimized.

@gziolo

gziolo May 22, 2018

Member

Should it be verified inside the setMobileViewPort method?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 23, 2018

Member

True the code was updated 👍

@@ -81,6 +108,13 @@ export async function getHTMLFromCodeEditor() {
return textEditorContent;
}
export async function clearLocalStorage() {
await page.$eval( 'body', ( body ) => {

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

We should be able to use page.evaluate here, which is run as within the context of the browser:

await page.evaluate( () => window.localStorage.clear() );

https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pageevaluatepagefunction-args

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 4, 2018

Member

Hi @aduth, thank you I applied this version and it works great.
Before I tried it and I had problems but I think the problem was related to other error that was meanwhile solved because now it works exactly as expected.

// eslint-disable-next-line no-undef
fail( error );
} );
export async function newMobileBrowserPage() {

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

Where is this function used?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 22, 2018

Member

It was not being used I added it to be consistent we had setMobileViewport, setDesktopViewport, and newDesktopBrowserPage so it made sense to create newMobileBrowserPage. I removed it for now if it is required later it can be easily added.

return window.innerWidth;
}
);
expect( width ).toBeLessThan( 782 );

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

Aren't we expecting this to be guaranteed by setMobileViewPort ?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 22, 2018

Member

Yes it should be guaranteed, I removed this check.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 22, 2018

Member

After the removal of this check the test sometimes started to fail. It turns out it was related with puppeteer issue GoogleChrome/puppeteer#1751.

I added a waitForFunction to address it.

export async function newDesktopBrowserPage() {
global.page = await browser.newPage();
await setDesktopViewPort();

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

Capitalization should be "Viewport", if even just for consistency with Puppeteer:

https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagesetviewportviewport

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 22, 2018

Member

Capitalization corrected, thank your for the catch.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 22, 2018

Hi @gziolo and @aduth thank you for your review, this PR is read for a new look.

@gziolo

gziolo approved these changes Jun 25, 2018

In general, it looks good and tests well. I had 2 minor comments where I feel we could remove some code duplications and increase readability of the code.

await setDesktopViewport();
}
export async function setDesktopViewport() {

This comment has been minimized.

@gziolo

gziolo Jun 25, 2018

Member

Can we generalize this method as follows:

export async function setViewport( type ) {
    const allowedDimensions = { desktop: { width: 1000, height: 700 } };
    const currentDimmension = allowedDimensions[ type ];
    await page.setViewport( currentDimmension );
    await waitForPageDimensions( currentDimmension.width, currentDimmension.height );
}

I'm wondering if also should align the names with what we use for the viewport module.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 25, 2018

Member

Feedback was addressed. Than you for the review!

} );
it( 'Should have the sidebar closed by default on mobile', async () => {
await setMobileViewport();

This comment has been minimized.

@gziolo

gziolo Jun 25, 2018

Member

Won't it give false positives because we are setting the desktop viewport first?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 25, 2018

Member

We are setting the desktop viewport first as the default viewport but Gutenberg is not open. We are only opening Gutenberg in the newPost call.

it( 'Should reopen sidebar the sidebar when resizing from mobile to desktop if the sidebar was closed automatically', async () => {
await newPost();
await setMobileViewport();

This comment has been minimized.

@gziolo

gziolo Jun 25, 2018

Member

Here we have a different order await setDesktopViewport(); and the new post is created and then changed to mobile. Maybe we should always set viewport in the test just before creating a new post for consistency?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 25, 2018

Member

Yes, I totally agree, in this test cases setting the viewport in the tests make things clear.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 25, 2018

Cool, thanks for iterating on this one 🚢

@jorgefilipecosta jorgefilipecosta merged commit e222ccb into master Jun 25, 2018

2 checks passed

codecov/project 46.76% remains the same compared to d7dc8f2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/end-to-end-tests-sidebar-bahaviours branch Jun 25, 2018

oxyc added a commit to generoi/gutenberg that referenced this pull request Jun 26, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg
* 'master' of https://github.com/WordPress/gutenberg: (69 commits)
  fix: Show permalink editor in editor (WordPress#7494)
  Fix text wrapping in Firefox. (WordPress#7472)
  Try another approach to fixing the sibling inserter in Firefox (WordPress#7530)
  fix: Improve "add block" text in NUX onboarding (WordPress#7511)
  Implement core style of including revisions data on Post response (WordPress#7495)
  Testing: Add e2e test for PluginPostStatusInfo (WordPress#7284)
  Add end 2 end test for sidebar behaviours on mobile and desktop. (WordPress#6877)
  Only save metaboxes when it's not an autosave (WordPress#7502)
  Fix broken links in documentation (WordPress#7532)
  Remove post type 'viewable' compatibility shim (WordPress#7496)
  Fix typo. (WordPress#7528)
  Blocks: Remove wrapping div from paragraph block (WordPress#7477)
  fix: change import for InnerBlocks (WordPress#7484)
  Polish library just a teeeeensy bit (WordPress#7522)
  feat: Add snapshot update script (WordPress#7514)
  Display server error message when one exists (WordPress#7434)
  Fix issues with gallery in IE11. (WordPress#7465)
  Polish region focus style (WordPress#7459)
  Fix IE11 formatting toolbar visibility (WordPress#7413)
  Update plugin version to 3.1. (WordPress#7402)
  ...

@jorgefilipecosta jorgefilipecosta added this to the 3.2 milestone Jul 5, 2018

@@ -0,0 +1,80 @@
/**

This comment has been minimized.

@aduth

aduth Oct 22, 2018

Member

-behaviour is kinda redundant as a suffix, and doesn't add much value. Any test could be called as such, undo-behavior.test.js, writing-flow-behavior.test.js, etc.

Also, typo since it's spelled "behavior", not "behaviour" :trollface: 🇺🇸

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