WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 weeks ago

#48978 new task (blessed)

Add a new test case for E2E test (Show admin bar if user logged in)

Reported by: hideokamoto Owned by:
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-testing needs-refresh
Focuses: javascript Cc:

Description

We can add a new test case for our e2e test.

Case

  • When: User logged in.
  • Where: Go to own website.
  • Expect: Should show admin bar top of the page

Attachments (5)

48978.patch (874 bytes) - added by hideokamoto 2 years ago.
To add the test case
48978-1.patch (820 bytes) - added by hideokamoto 22 months ago.
Update patch regarding to the feedback
48978-2.patch (817 bytes) - added by hideokamoto 21 months ago.
New patch file for the ticket
48978-3.patch (947 bytes) - added by justinahinon 7 weeks ago.
48978-4.patch (1.1 KB) - added by justinahinon 7 weeks ago.

Download all attachments as: .zip

Change History (18)

@hideokamoto
2 years ago

To add the test case

#1 @hideokamoto
2 years ago

  • Type changed from defect (bug) to enhancement

#2 @talldanwp
22 months ago

  • Keywords needs-patch added

Hi @hideokamoto, Thanks for adding an end to end test. I thought I'd share some feedback about the test:

  • It'd be great if you could add semi-colons as per the coding standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#semicolons)
  • Currently there's a separate import for loginUser, this should ideally be part of import on the line above for e2e-test-utils so that it looks like this:
    import { visitAdminPage, createURL, loginUser } from '@wordpress/e2e-test-utils';
    
  • expect( nodes.length ).not.toEqual( 0 ); I know the test above has a similar expectation, but as far as I'm aware more than one admin bar would be unexpected. I think it'd be better to change this to expect( nodes.length ).toBe( 1 );, it's also safe to use the stricter toBe matcher for numbers.
  • I think it could be worth also testing for absence of the admin bar when logged out, this would likely be a separate test case.
  • What do you think about changing the test description from 'Login User' to 'Admin Bar'? I think that would better reflect what's being tested.

Thanks!

@hideokamoto
22 months ago

Update patch regarding to the feedback

#3 @hideokamoto
22 months ago

Thanks for the feedback!
I've updated the patch today.

expect( nodes.length ).not.toEqual( 0 ); I know the test above has a similar expectation, but as far as I'm aware more than one admin bar would be unexpected. I think it'd be better to change this to expect( nodes.length ).toBe( 1 );, it's also safe to use the stricter toBe matcher for numbers.

I've replaced the assertion from expect( nodes.length ).not.toEqual( 0 ) to expect( ! ! ( nodes ) ).toBe( true ).

Because, the assertion expect( nodes.length ).toBe( 1 ) not works well.

> node ./tests/e2e/run-tests.js

 FAIL  tests/e2e/specs/hello.test.js
  Hello World
    ✓ Should load properly (1814ms)
  Admin Bar
    ✕ Should show admin bar when user logged in (1021ms)

  ● Admin Bar › Should show admin bar when user logged in

    TypeError: Cannot read property 'length' of null

      15 |              await page.goto( createURL( '/' ) );
      16 |              const nodes = await page.$( '#wpadminbar1' );
    > 17 |              expect( nodes.length ).toBe( 1 );
         |                            ^
      18 |      } );
      19 | } );

      at Object.<anonymous> (specs/hello.test.js:17:17)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Snapshots:   0 total
Time:        4.018s
Ran all test suites.

So I change the code to check just the element exists.

Pass case

describe( 'Admin Bar', () => {
	it( 'Should show admin bar when user logged in' , async () => {
		await loginUser();
		await page.goto( createURL( '/' ) );
		const nodes = await page.$( '#wpadminbar' );
		expect( ! ! ( nodes ) ).toBe( true );
	} );
} );

  Admin Bar
    ✓ Should show admin bar when user logged in (1041ms)

Failure case

describe( 'Admin Bar', () => {
	it( 'Should show admin bar when user logged in' , async () => {
		await loginUser();
		await page.goto( createURL( '/' ) );
		const nodes = await page.$( '#wpadminbar111' );
		expect( ! ! ( nodes ) ).toBe( true );
	} );
} );

  Admin Bar
    ✕ Should show admin bar when user logged in (802ms)

  ● Admin Bar › Should show admin bar when user logged in

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      15 |              await page.goto( createURL( '/' ) );
      16 |              const nodes = await page.$( '#wpadminbar111' );
    > 17 |              expect( ! ! ( nodes ) ).toBe( true );
         |                                      ^
      18 |      } );
      19 | } );

      at Object.<anonymous> (specs/hello.test.js:17:27)

I think it could be worth also testing for absence of the admin bar when logged out, this would likely be a separate test case.

I think so too.
But currently, there is a no helper function to logout the dashboard.
So probably, we need to create a helper function to logout the dashboard.


Kindly regards.

#4 @talldanwp
22 months ago

Hi @hideokamoto,

Thanks for updating the patch. I see what's happening. It looks like the reason checking the length is not working is due to the selector using the page.$ function, which is the equivalent of document.querySelector and only returns the first element it encounters.

If you use page.$$ (document.querySelectorAll) instead the test seems to work well:

it( 'Should show admin bar when user logged in' , async () => {
	await loginUser();
	await page.goto( createURL( '/' ) );
	const nodes = await page.$$( '#wpadminbar' );
	expect( nodes.length ).toBe( 1 );
} );

This ticket was mentioned in Slack in #hosting-community by mike. View the logs.


21 months ago

@hideokamoto
21 months ago

New patch file for the ticket

#6 @hideokamoto
21 months ago

@talldanwp
I'm sorry for the late reply.

I've tried your code and it works fine!
I've updated the patch now.

Thanks.

#7 @hideokamoto
11 months ago

any update?

#8 @justinahinon
7 weeks ago

Hello @hideokamoto,

Thanks for opening the ticket and the work you did on it.

I have updated your last patch 48978-2.patch​ and make a couple of updates/additions on it in https://core.trac.wordpress.org/attachment/ticket/48978/48978-4.patch.

  • For the first test (checking if the admin bar is present), you don't need to use these two lines to log in as an admin and access the dashboard.
await loginUser();
await page.goto( createURL( '/' ) );

You can simply use await visitAdminPage( '/' ); (see https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-test-utils/src/visit-admin-page.js) that will visit the admin dashboard and log in as an admin if it is not the case.

  • I then removed the loginUser() utility function as it was not used anymore
  • I added another test that checks that the admin bar is not present when not logged in.

#9 @justinahinon
7 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Awaiting Review to 5.9

cc @isabel_brison @hellofromtonya @kevin940726 this e2e test ticket has a patch and will need review :).

#10 @isabel_brison
6 weeks ago

Ran the tests locally and they are working well.

One minor typo: "Should not show admin bar"

And we should move these tests to a separate "admin-bar.test.js" file (or some similar name). "Hello World" was essentially a dummy test to make sure the e2e setup was working correctly; now that we have actual tests in there we could even remove it altogether :)

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


4 weeks ago

#12 @hellofromTonya
3 weeks ago

  • Type changed from enhancement to task (blessed)

#13 @hellofromTonya
2 weeks ago

  • Keywords needs-refresh added
  • Milestone changed from 5.9 to 6.0

As today is 5.9 Beta 1, moving this to 6.0. Tests are not strictly bound to a milestone. Once the patch is approved and ready, it can committed.

Note: See TracTickets for help on using tickets.