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

[GTIN] Adding more tests for GTIN migration #2679

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Nov 18, 2024

Changes proposed in this Pull Request:

Closes part of #2617

  • This PR adds tests specifically for the GTIN Migration
  • Additionally, this PR adds tests for GTIN Hidden feature
  • A bug was found when writing the tests (Core GTIN was overridden in the migration). This PR fixes the bug.

Detailed test instructions:

  1. See the tests passing
  2. Create a simple product and set a GTIN in Product Inventory tab
  3. Set also the GTIN in Google for the WooCommerce tab
  4. Start the migration in Connection test page (Start GTIN Migration)
  5. See that the GTIN was not overridden

Additional details:

Changelog entry

Dev - Adding tests for GTIN migration tool

@puntope puntope requested a review from a team November 18, 2024 15:39
@puntope puntope self-assigned this Nov 18, 2024
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Nov 18, 2024
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.2%. Comparing base (66dfa38) to head (6910e69).
Report is 9 commits behind head on add/support-core-gtin-field.

Additional details and impacted files

Impacted file tree graph

@@                       Coverage Diff                       @@
##             add/support-core-gtin-field   #2679     +/-   ##
===============================================================
+ Coverage                           64.9%   65.2%   +0.3%     
- Complexity                          4670    4671      +1     
===============================================================
  Files                                478     478             
  Lines                              19529   19532      +3     
===============================================================
+ Hits                               12681   12735     +54     
+ Misses                              6848    6797     -51     
Flag Coverage Δ
php-unit-tests 65.2% <100.0%> (+0.3%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Jobs/MigrateGTIN.php 88.6% <100.0%> (+88.6%) ⬆️

... and 2 files with indirect coverage changes

---- 🚨 Try these New Features:

@puntope puntope marked this pull request as ready for review November 18, 2024 15:44
Copy link
Contributor

@mikkamp mikkamp left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests. They look good to me and they all pass as expected.

Based on the report here I noticed that we do not test migrating GTIN's from variable products, nor when an exception is thrown such as a duplicate GTIN.

Since migration is temporary I'm not too worried that this doesn't have full test coverage, but maybe we can at least log an issue with the missing tests.

@puntope puntope merged commit 25d85de into add/support-core-gtin-field Nov 21, 2024
12 checks passed
@puntope puntope deleted the dev/tests-gtin branch November 21, 2024 14:10
@ianlin ianlin mentioned this pull request Nov 26, 2024
22 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.

2 participants