Skip to content

Conversation

@tyxla
Copy link
Member

@tyxla tyxla commented Jul 18, 2019

Currently, WooCommerce tests are broken. We tried to add some test cases in #13079, but we were unable to run the tests.

The reason seems to be simple: our WooCommerce tests have a hardcoded bit where they load files from the WooCommerce test framework (borrowed from their unit test bootstrap), and some of the file names were obsolete.

This PR fixes those by using the includes from the latest WooCommerce master.

Changes proposed in this Pull Request:

  • Unit Tests: Fix WooCommerce tests bootstrap

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Needed for #13079.

Testing instructions:

  • Checkout this branch.
  • Make sure you have the latest WooCommerce git repo checked out in /docker/wp-content/plugins/woocommerce, and WooCommerce is built (composer install, npm install, npm run build in the woo dir does the job).
  • Navigate to the Jetpack repo root.
  • Run yarn docker:sh
  • While in the shell, run cd wp-content/plugins/jetpack
  • Run JETPACK_TEST_WOOCOMMERCE=1 phpunit --filter WP_Test_Jetpack_Sync_WooCommerce
  • Verify tests work and pass ✅

Proposed changelog entry for your changes:

  • Unit Tests: Fix WooCommerce tests bootstrap

@tyxla tyxla added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. Unit Tests labels Jul 18, 2019
@tyxla tyxla added this to the 7.6 milestone Jul 18, 2019
@tyxla tyxla requested review from a team July 18, 2019 09:01
@tyxla tyxla self-assigned this Jul 18, 2019
@tyxla tyxla requested a review from timmyc July 18, 2019 09:01
@jetpackbot
Copy link
Collaborator

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 11894a6

Copy link
Contributor

@roccotripaldi roccotripaldi left a comment

Choose a reason for hiding this comment

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

I get this error when running the tests:

Fatal error: Declaration of WC_Unit_Test_Case::assertNotFalse($condition, $message = '') must be compatible with WP_UnitTestCase::assertNotFalse($condition, string $message = ''): void in /var/www/html/wp-content/plugins/woocommerce/tests/framework/class-wc-unit-test-case.php on line 145

I think it's not related to this PR, I can bypass the error by deleting Woo's declaration of assertNotFalse and then all Jetpack Woo tests pass. I say we ship it!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 18, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Same as Rocco (discussion in p1563462564245100-slack-jetpack-crew). Let's merge this!

@roccotripaldi roccotripaldi merged commit 7f40e97 into master Jul 18, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 18, 2019
@roccotripaldi roccotripaldi deleted the fix/woocommerce-unit-tests-includes branch July 18, 2019 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug When a feature is broken and / or not performing as intended Unit Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants