search

Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
87 stars 32 forks source link

[Epic] Automated Checks #32

Open brucealdridge opened 2 years ago

brucealdridge commented 2 years ago

Description

Add automated checks to pull requests to make submissions and reviewing easier.

Checks to add

haszari commented 2 years ago

@mattallan Interested in your thoughts on what checks are most useful/essential for subs-core. Also the relationship to other repos: what checks do we get for free from WCPay or subs pro repo.

haszari commented 2 years ago

Pinging in @aprea since you're our DRI for patch release follow up. This issue could address one of the patch follow up items - can you confirm what's most urgent here with @mattallan. We can iterate and add more checks in future, for now the most important thing is to get something in place to cover gaps identified in 3.2.x. cc @brucealdridge

mattallan commented 2 years ago

Interested in your thoughts on what checks are most useful/essential for subs-core.

I think the most important thing is to get some PHP linting checks for the various PHP version. This was the major breaking point we saw when releasing 3.2 and is probably one of the easier things to check so I would say having that is a good starting point!

I'm not too fussed on other easy things to add like psalm at this point.

Also the relationship to other repos: what checks do we get for free from WCPay or subs pro repo.

In WC Payments most of their checks ignore the subscriptions-core files so we don't get anything for free there. Maybe the E2E tests in wcpay will technically have WCS Core enabled 🤔

We can iterate and add more checks in future, for now the most important thing is to get something in place to cover gaps

Yeah, I definitely agree with this!

Here's my 2c on how I would order/prioritize these:

  1. PHP linting (implemented with #34)
  2. PHPCS running on new changes (implemented with #34)
  3. JS linting
  4. Have some of these checks run on pre-commit (similar to WC Payments)
  5. PHPUnit added and a simple assertTrue(true) unit test running
  6. Start requiring PHPUnit tests on any new functions
  7. Port/migrate any old unit tests from WC Subscriptions (these are old/slow tests so it might be worth looking into them and deciding if we should scrap them entirely or make them more efficient as we copy them over)
  8. Basic e2e test ?
  9. Start updating existing code to run on the latest WC codesniffer rules
  10. Add any extra checks

I'm probably missing something obvious in that list, but what's everyone's thoughts on that or?

aprea commented 2 years ago

That looks like a solid list @mattallan, I'm happy with that as a starting point 👍

brucealdridge commented 2 years ago

34 marked this as fixed but I'm going to reopen this and add list from @mattallan to the top comment so this can keep track everything. Feel free to close and split of into multiple issues if that is easier

shendy-a8c commented 2 years ago

This is great, @brucealdridge! I just added to the bottom of the list to enforce code coverage, something similar to https://github.com/Automattic/woocommerce-payments/pull/2342.

james-allan commented 2 years ago

Just noting that I've moved this back to the in-progress pipeline now that the first PR was merged. But feel free to push it back if this isn't something you're looking into.

haszari commented 2 years ago

Just noting that I've moved this back to the in-progress pipeline now that the first PR was merged. But feel free to push it back if this isn't something you're looking into.

+1, happy for this to go to Icebox or New issues while we're focused on sprint & projects. We can iterate on this over time.

haszari commented 2 years ago

Shifting this to Icebox now we're out of cooldown - though we've made some great progress here!

aprea commented 2 years ago

Hey @brucealdridge, FYI, I have checked the JS linting and Have some of these checks run on pre-commit (similar to WC Payments) checkboxes in the PR description.

I believe these were completed in #57?

Apologies if that's not the case and feel free to uncheck them if there's something pending.

haszari commented 1 year ago

Marking as low – definitely keen for improvements here but this is an internal task (not impacting merchants).