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

[hideokamoto]

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

[hideokamoto]

To add the test case

[talldanwp]

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]

Update patch regarding to the feedback

[hideokamoto]

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.

[talldanwp]

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 );
} );

[hideokamoto]

New patch file for the ticket

[hideokamoto]

@talldanwp
I'm sorry for the late reply.

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

Thanks.

[hideokamoto]

any update?

[justinahinon]

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.

[justinahinon]

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

[isabel_brison]

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 :)

[hellofromTonya]

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.

[kirasong]

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.

[hellofromTonya]

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.

[chaion07]

@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

[desrosj]

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

[hellofromTonya]

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.

[hellofromTonya]

Clearing me as the ticket owner, as I'm no longer am a sponsored contributor. Clearing ownership opens the ticket for someone to step in to shepherd this ticket forward to resolution.

Trac ticket: https://core.trac.wordpress.org/ticket/48978

[SirLouen]

I can't see a whole new file and unit test just to test this.

e2e tests are super-costly in terms of resources

I would rather add this test inside another relevan ttest (there are a good amount of tests that can test this) or at minimum I would add it inside the file of another existing test.

I've been unable to find which are the recommended guidelines for e2e on WP project.

Maybe Gutenberg active members can give you some guidance about this
cc @Mamaduka @wildworks

[wildworks]

I'm not familiar with E2E tests in the core, but it seems that only very basic tests exist currently.

The overall situation of the project may have changed since this patch was submitted 5 years ago, so it might be a good idea to discuss what kind of E2E tests should be added to the core.

Personally, I think it's fine to move forward with this patch. There are ​already unit tests for the admin bar, but there are no tests that correspond to the logged-in state, and I think E2E tests are ideal for testing the display state on the front end.