Skip to content
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 admin-e2e-tests #983

Closed
wants to merge 2 commits into from
Closed

Add admin-e2e-tests #983

wants to merge 2 commits into from

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Aug 28, 2021

Changes proposed in this Pull Request:

Add admin-e2e-tests
to make sure our plugin does not break WooCommerce Admin.
Add @woocommerce/admin-e2e-tests dependency,
clone test suites from https://github.com/woocommerce/woocommerce/tree/trunk/tests/e2e/specs as suggested in the release post (pcShBQ-6A-p2).

It seems valuable, as we actually fail on two:

 FAIL  ../../../tests/e2e/specs/wc-admin/admin-analytics/analytics-overview.test.js (37.502s)
  Analytics pages
    ✓ a user should see 3 sections by default - Performance, Charts, and Leaderboards (157ms)
    ✓ should allow a user to remove a section (248ms)
    ✓ should allow a user to add a section back in (199ms)
    moving sections
      ✕ should not display move up for the top, or move down for the bottom section (95ms)
      ✕ should allow a user to move a section down (30012ms)
      ✕ should allow a user to move a section up (203ms)

Screenshots:

image

Detailed test instructions:

  1. Checkout this branch
  2.  npm i
     npm run docker:up
     npm run test:e2e

Changelog entry

Add - WooCommerce Admin tests suite.

to make sure our plugin does not break WooCommerce Admin.
Add `@woocommerce/admin-e2e-tests` dependency,
clone test suites from https://github.com/woocommerce/woocommerce/tree/trunk/tests/e2e/specs as suggested in the release post (pcShBQ-6A-p2).
@tomalec
Copy link
Member Author

tomalec commented Aug 28, 2021

The tests fail due to admin-e2e expecting "Mode down" but get "Move Down", so it seems to be another dependency version mismatch.

@tomalec tomalec requested review from rrennick and a team and removed request for a team August 28, 2021 11:50
Copy link

@rrennick rrennick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomalec This looks fine to me. They tested great locally. We have an issue to investigate why the analytics overview tests fail in some Mac environments.

  Analytics pages
    ✓ a user should see 3 sections by default - Performance, Charts, and Leaderboards (110ms)
    ✓ should allow a user to remove a section (153ms)
    ✕ should allow a user to add a section back in (548ms)
    moving sections
      ✓ should not display move up for the top, or move down for the bottom section (139ms)
      ✕ should allow a user to move a section down (521ms)
      ✕ should allow a user to move a section up (30011ms)

@tomalec
Copy link
Member Author

tomalec commented Aug 31, 2021

Thank you @rrennick !


Now, there is a question for the Woogle team. Those tests require 5.6, but we are quite behind:

 * WC requires at least: 5.2
 * WC tested up to: 5.5
  • Do we want to merge it?
  • Do we want to skip failing ones or keep them failing? - They are simply failing due to the older version of WC, so there is nothing actionable on our end.

Currently, we are not running E2E on CI, so broken tests would not block anything.

@tomalec tomalec marked this pull request as ready for review August 31, 2021 21:44
@tomalec tomalec requested a review from a team August 31, 2021 21:44
@ecgan
Copy link
Member

ecgan commented Sep 1, 2021

we are quite behind:

I think we would need to test and bump it up one day. Latest version of WooCommerce is 5.6.0 released about two weeks ago, so we should bump it to the following ("at least" version is L-2):

 * WC requires at least: 5.4
 * WC tested up to: 5.6

Those tests require 5.6...

  • Do we want to merge it?

I think we would need to take the "lowest common denominator" approach (see previous related Slack thread in our team channel p1629975717010800/1629914514.272300-slack-CK365S85V) and make sure things work with L-2 version, so we probably shouldn't merge it now, but later when "at least" version becomes 5.6.0.

@tomalec
Copy link
Member Author

tomalec commented Sep 1, 2021

I think we would need to take the "lowest common denominator" approach (see previous related Slack thread in our team channel p1629975717010800/1629914514.272300-slack-CK365S85V) and make sure things work with L-2 version, so we probably shouldn't merge it now, but later when "at least" version becomes 5.6.0.

I find it funky.
As I've seen admin-e2e-tests a way to check:

  • whether our plugin does not break the core WC features
  • whether we are compatible with the given version of WC

And from the results of the tests, you can tell, that we did not break a thing, and our plugin is compatible with 5.6, but we cannot use it before we bump our minimal version to 5.6. As what they check:

  • whether the minimal WC version used by our plugin is compatible with the tests for the latest version of WC - which is not, as there was a small change in WC.

It would be nice to have a package that once used would run the tests for a WC version against that version of WC.

@tomalec tomalec added type: technical debt - dependency extraction This issue/PR suffers from the dependency extraction and management type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies labels Sep 16, 2021
@tomalec
Copy link
Member Author

tomalec commented Jul 11, 2022

This PR, grown old, and rotten a bit, I'll try to redo it on the latest develop.
Plus, given jestjs/jest#11781 is fixed, now I'd try to make it so we will run the test from node_modules directly, without the need for duplication, and wrapping increasing the risk of timeout.

@mikkamp
Copy link
Contributor

mikkamp commented Feb 12, 2024

Our E2E tests no longer rely on the packages from WooCommerce as they are deprecated. We use wp-env now, see PR #2030
So I'm closing this PR.

@mikkamp mikkamp closed this Feb 12, 2024
@eason9487 eason9487 deleted the add/admin-e2e branch February 27, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: technical debt - dependency extraction This issue/PR suffers from the dependency extraction and management type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants