site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Replace unnecessary Key Metrics VRT scenarios with Jest test when appropriate

Open kuasha420 opened this issue 1 year ago • 3 comments

Feature Description

Each and every Key Metrics widget stories also includes all their stories in Visual Regression suite. This is repetitive and unnecessarily increases the VRT running time on CI and bloats the suite. This includes:

  • Ready
  • ReadyViewOnly
  • Loading
  • ZeroData
  • GatheringData / DataUnavailable
  • Error
  • ErrorMissingCustomDimensions
  • ErrorCustomDimensionsInsufficientPermissions
  • ErrorCustomDimensionsGeneric
  • InsufficientPermissions

The stories for each title should only keep the generic Ready, ReadyViewOnly and any other unique stories for them. ie. AdSenseNotLinked story for TopEarningContentWidget. Besides the unique stories, VRT scenarios should be removed from the generic stories as well. (ie. Ready. ReadyViewOnly).

Instead, the generic VRT scenarios should be added to the actual underlying tile components. ie. MetricTileWrapper, MetricTileText, MetricTileTable, MetricTileNumeric etc.

Specific conditions for a Key Metrics widgets (ie. ErrorMissingCustomDimensions, Error etc) should be verified in component Jest test instead, wherever applicable.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • All non-unique, redundant VRT scenarios for all KMW tiles should be removed. These include:
    • Zero
    • Loading
    • Error states
    • InsufficientPermissions
  • VRT scenarios for Loading, Error and Insufficient Permissions states should be added only once within a common component story.
  • VRT scenarios for Zero states should be added for each of the three metric tile types: MetricTileText, MetricTileTable, MetricTileNumeric.
  • When removing VRT scenarios, check if suitable Jest Test coverage could be migrated to reproduce actual error conditions.

Implementation Brief

  • [x] In assets/js/modules/analytics-4/components/widgets for each KMW:
    • Remove usage of .scenario for scenarios listed in the AC
    • With following exceptions, for:
      • PopularAuthorsWidget.stories.js, PopularProductsWidget.stories.js, TopCategoriesWidget.stories.js, TopRecentTrendingPagesWidget.stories.js
        • They have additional scenarios, Gathering Data and Error - Custom dimensions creation - Insufficient Permissions, Error - Custom dimensions creation - Generic. Remove them as well, and transfer these to jest tests. These components currently do not have jest tests, so they need to be created.
        • Keep these scenarios included, only for 1 widget, for example PopularAuthorsWidget.stories.js
  • [x] In assets/js/modules/adsense/components/widgets/TopEarningContentWidget.stories.js * It has AdSense Not Linked unique scenario which can be kept, and others as listed in AC should be removed
  • [x] assets/js/modules/search-console/components/widgets/PopularKeywordsWidget.stories.js
    • Has no unique scenarios, and they can be removed per AC list
  • For the scenarios that are kept, remove unnecessary usage of delay property
  • Zero story scenario along with others, are already covered in MetricTileText, MetricTileTable, and MetricTileNumeric, no changes are needed in that regard. Just remove the delay property from existing scenarios

Test Coverage

  • Covered in the IB

QA Brief

  • No production code has been modified here - so this shouldn't require any QA.
  • QA:Eng can be done by the Merge Reviewer to ensure CI checks are passing - especially VRTs.

Changelog entry

  • N/A

kuasha420 avatar Mar 22 '24 05:03 kuasha420

Possible ticket for 132, based on the conversation during standup on 26/06

binnieshah avatar Jun 26 '24 14:06 binnieshah

@zutigrm IB LGTM ✅ Moving to EB

10upsimon avatar Oct 04 '24 06:10 10upsimon

Moving directly to Approval as no QA is needed here.

tofumatt avatar Oct 21 '24 22:10 tofumatt