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 E2E tests for the integration in the classic product editor #2363

Merged
merged 17 commits into from
Apr 15, 2024

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Apr 11, 2024

Changes proposed in this Pull Request:

  • Add menu_order properties to the variations data in tests/e2e/config/default.json to make them can be updated correctly via the classic product editor.
  • Add E2E tests

📌 Checklist (@eason9487)

  • Revert 253f16c before merging this PR

Detailed test instructions:

  1. npm run dev to build files
  2. npm run test:e2e to see if the E2E test in headless mode can pass.
  3. (Optional) npm run test:e2e -- --ui to inspect the E2E test in UI mode.
  4. View the E2E test result on GitHub Actions: https://github.com/woocommerce/google-listings-and-ads/actions/runs/8645102656/job/23701605350?pr=2363

Changelog entry

@eason9487 eason9487 self-assigned this Apr 11, 2024
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Apr 11, 2024
@eason9487 eason9487 requested a review from a team April 11, 2024 10:25
@eason9487 eason9487 marked this pull request as ready for review April 11, 2024 10:26
Copy link
Contributor

@martynmjones martynmjones left a comment

Choose a reason for hiding this comment

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

Hey @eason9487, thanks for expanding the E2E tests to cover the classic product editor.

Tested the branch locally and confirmed tests are passing and expectations all look correct so LGTM ✅

return page.locator( '.gla_attributes_multipack_field input' );
},

getAllProductAttributes( locator = page ) {
Copy link
Contributor

@puntope puntope Apr 13, 2024

Choose a reason for hiding this comment

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

I see this function is almost the same as the getProductBlockEditorUtils version. Can we refactor it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted in 8a79933.

Comment on lines 133 to 134
const variableId = await api.createVariableProduct();
await api.createVariationProducts( variableId );
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be refactored as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted in 8a79933.

adultContent,
} = editorUtils.getAllProductAttributes( locator );

const allPairs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated with block-integration equivalent. Can we refactor it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted in f21dd78.

Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

Thanks @eason9487 for your work here.

The tests are passing and the code looking good for me.

I left some comments regarding possible refactors and preventing code duplication.

Approved in advance

@eason9487 eason9487 merged commit 33c35a8 into develop Apr 15, 2024
5 checks passed
@eason9487 eason9487 deleted the dev/e2e-classic-product-editor branch April 15, 2024 04:14
@ianlin ianlin mentioned this pull request Apr 16, 2024
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants