Make WordPress Core

Opened 4 years ago

Last modified 18 months ago

#48978 assigned defect (bug)

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

Reported by: hideokamoto's profile hideokamoto Owned by: hellofromtonya's profile hellofromTonya
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-testing has-patch changes-requested
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 4 years ago.
To add the test case
48978-1.patch (820 bytes) - added by hideokamoto 4 years ago.
Update patch regarding to the feedback
48978-2.patch (817 bytes) - added by hideokamoto 4 years ago.
New patch file for the ticket
48978-3.patch (947 bytes) - added by justinahinon 2 years ago.
48978-4.patch (1.1 KB) - added by justinahinon 2 years ago.

Download all attachments as: .zip

Change History (26)

@hideokamoto
4 years ago

To add the test case

#1 @hideokamoto
4 years ago

  • Type changed from defect (bug) to enhancement

#2 @talldanwp
4 years 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
4 years ago

Update patch regarding to the feedback

#3 @hideokamoto
4 years 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
4 years 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.


4 years ago

@hideokamoto
4 years ago

New patch file for the ticket

#6 @hideokamoto
4 years 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
3 years ago

any update?

#8 @justinahinon
2 years 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
2 years 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
2 years 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.


2 years ago

#12 @hellofromTonya
2 years ago

  • Type changed from enhancement to task (blessed)

#13 @hellofromTonya
2 years 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.

This ticket was mentioned in Slack in #core by mike. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by mike. View the logs.


2 years ago

#16 @kirasong
2 years ago

  • Keywords needs-patch added; has-patch needs-testing needs-refresh removed

Folks present checked into this ticket during a bug scrub today.

It looks like the patch still applies, so removing needs-refresh.

There were a couple of changes suggested in https://core.trac.wordpress.org/ticket/48978#comment:10, so swapping has-patch for needs-patch.

As it's a ticket with only tests, it can land whenever during the cycle.

#17 @hellofromTonya
23 months ago

  • Milestone changed from 6.0 to 6.1
  • Owner set to hellofromTonya
  • Status changed from new to assigned

6.0 RC1 is tomorrow. Though this is a test ticket, there is still work to do. Moving it to 6.1 and assigning myself as owner to help shepherd it forward.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


18 months ago

#19 @chaion07
18 months ago

  • Keywords needs-testing added

@hideokamoto thank you for reporting this ticket. We reviewed this ticket during a recent bug-scrub session. Here's the summary:

  1. We feel the need for a proper testing
  2. The dummy test codes can be removed
  3. Added keyword: needs-testing

Cheers!

Props to @priyomukul

#20 @desrosj
18 months ago

  • Keywords has-patch changes-requested added; needs-patch removed

Re-adding has-patch and adding the new changes-requested.

#21 @hellofromTonya
18 months ago

  • Milestone changed from 6.1 to Future Release
  • Type changed from task (blessed) to defect (bug)

6.1 RC1 is tomorrow. Changes are requested. As there's been no updates to the patches and the ticket has been punted multiple times, moving it to Future Release. Once the changes are made, then please move it into the current alpha/beta milestone.

Also, please create a PR for the updated patch to let the new test(s) run through the automated CI workflows.

Note: See TracTickets for help on using tickets.