WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 8 months ago

#48978 new enhancement

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

Reported by: hideokamoto Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
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 (3)

48978.patch (874 bytes) - added by hideokamoto 11 months ago.
To add the test case
48978-1.patch (820 bytes) - added by hideokamoto 9 months ago.
Update patch regarding to the feedback
48978-2.patch (817 bytes) - added by hideokamoto 8 months ago.
New patch file for the ticket

Download all attachments as: .zip

Change History (9)

@hideokamoto
11 months ago

To add the test case

#1 @hideokamoto
11 months ago

  • Type changed from defect (bug) to enhancement

#2 @talldanwp
9 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
9 months ago

Update patch regarding to the feedback

#3 @hideokamoto
9 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
8 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.


8 months ago

@hideokamoto
8 months ago

New patch file for the ticket

#6 @hideokamoto
8 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.

Note: See TracTickets for help on using tickets.