Khadreal commented 3 months ago

Description

Add thorw error check to wp command Fixes #103

Documentation

User documentation

N/A

Technical documentation

With the new changes, if no error is thrown from wp command then the command ran successfully

Explain how this code works. Diagram & drawings are welcomed.

Type of change

[x] Enhancement (non-breaking change which improves an existing functionality).

New dependencies

List any new dependencies that are required for this change.

Risks

List possible performance & security issues or risks, and explain how they have been mitigated.

Checklists

Feature validation

[x] I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
[x] I triggered all changed lines of code at least once without new errors/warnings/notices.
[ ] I implemented built-in tests to cover the new/changed code.

Documentation

[ ] I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
[ ] The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
[ ] I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

[x] I wrote self-explanatory code about what it does.
[x] I wrote comments to explain why it does it.
[x] I named variables and functions explicitely.
[x] I did not introduce unecessary complexity.

Observability

[x] I handled errors when needed.
[ ] I wrote user-facing messages that are understandable and provide actionable feedbacks.
[ ] I prepared ways to observe the implemented system (logs, data, etc.).

MathieuLamiot commented 3 months ago

@Khadreal I can't make this PR run on mathieu.e2e website. It works well with trunk. After checking a bit, adding a log in the condition you added checking response code:

Error in Command : wp plugin uninstall --deactivate force-wp-mobile --path=/var/www/html/mathieu.e2e.rocketlabsqa.ovh/ Fatal error: Uncaught UnexpectedValueException: The stream or file "/var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/wp-rocket-config/wp-rocket-debug.log.html" could not be opened in append mode: Failed to open stream: Permission denied in /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/Monolog/Handler/StreamHandler.php:115
Stack trace:
#0 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Logger/StreamHandler.php(51): WP_Rocket\Dependencies\Monolog\Handler\StreamHandler->write()
#1 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/Monolog/Handler/AbstractProcessingHandler.php(39): WP_Rocket\Logger\StreamHandler->write()
#2 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/Monolog/Logger.php(344): WP_Rocket\Dependencies\Monolog\Handler\AbstractProcessingHandler->handle()
#3 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/Monolog/Logger.php(642): WP_Rocket\Dependencies\Monolog\Logger->addRecord()
#4 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Logger/Logger.php(68): WP_Rocket\Dependencies\Monolog\Logger->info()
#5 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/classes/Buffer/class-abstract-buffer.php(120): WP_Rocket\Logger\Logger::info()
#6 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Engine/Optimization/Buffer/Optimization.php(41): WP_Rocket\Buffer\Abstract_Buffer->log()
#7 [internal function]: WP_Rocket\Engine\Optimization\Buffer\Optimization->__construct()
#8 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Definition/Definition.php(225): ReflectionClass->newInstanceArgs()
#9 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Definition/Definition.php(181): WP_Rocket\Dependencies\League\Container\Definition\Definition->resolveClass()
#10 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Definition/Definition.php(160): WP_Rocket\Dependencies\League\Container\Definition\Definition->resolveNew()
#11 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Definition/DefinitionAggregate.php(79): WP_Rocket\Dependencies\League\Container\Definition\Definition->resolve()
#12 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Container.php(175): WP_Rocket\Dependencies\League\Container\Definition\DefinitionAggregate->resolve()
#13 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Container.php(118): WP_Rocket\Dependencies\League\Container\Container->resolve()
#14 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Engine/Optimization/ServiceProvider.php(53): WP_Rocket\Dependencies\League\Container\Container->get()
#15 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/ServiceProvider/ServiceProviderAggregate.php(71): WP_Rocket\Engine\Optimization\ServiceProvider->register()
#16 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Container.php(192): WP_Rocket\Dependencies\League\Container\ServiceProvider\ServiceProviderAggregate->register()
#17 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/League/Container/Container.php(118): WP_Rocket\Dependencies\League\Container\Container->resolve()
#18 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Plugin.php(166): WP_Rocket\Dependencies\League\Container\Container->get()
#19 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/main.php(47): WP_Rocket\Plugin->load()
#20 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-includes/class-wp-hook.php(324): rocket_init()
#21 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#22 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-includes/plugin.php(517): WP_Hook->do_action()
#23 /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-settings.php(550): do_action()
#24 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1374): require('...')
#25 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1293): WP_CLI\Runner->load_wordpress()
#26 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(28): WP_CLI\Runner->start()
#27 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/bootstrap.php(83): WP_CLI\Bootstrap\LaunchRunner->process()
#28 phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php(32): WP_CLI\bootstrap()
#29 phar:///usr/local/bin/wp/php/boot-phar.php(20): include('...')
#30 /usr/local/bin/wp(4): include('...')
#31 {main}
  thrown in /var/www/html/mathieu.e2e.rocketlabsqa.ovh/wp-content/plugins/wp-rocket/inc/Dependencies/Monolog/Handler/StreamHandler.php on line 115
Error: There has been a critical error on this website.Learn more about troubleshooting WordPress. There has been a critical error on this website.

I think this is a permission issue on the server configuration. But for now, it means your PR breaks our Rocket-E2E workflow. Did you try it on your own e2e website? Do you have this behavior?

Also (don't know if this is related to the above or not), on your PR, the first test of test:lcp run fails:

1) Scenario: When I visited a page that has LCP with relative image # src/features/lcp-beacon-image.feature:10
   ✔ Before # src/support/hooks.ts:132
   ✔ Given I am logged in # src/support/steps/general.ts:23
   ✔ And plugin is installed 'new_release' # src/support/steps/general.ts:29
   ✔ And plugin is activated # src/support/steps/general.ts:48
   ✔ And I go to 'wp-admin/options-general.php?page=wprocket#dashboard' # src/support/steps/general.ts:93
   ✔ When I log out # src/support/steps/general.ts:136
   ✖ Then lcp image in 'lcp_regular_image_template' has fetchpriority # src/support/steps/lcp-beacon-script.ts:111
       Error: expect(received).toBeTruthy()

       Received: false
           at Proxy.<anonymous> (/Users/mathieu/Documents/Github/wp-rocket-e2e/node_modules/@playwright/test/lib/matchers/expect.js:165:37)
           at World.<anonymous> (/Users/mathieu/Documents/Github/wp-rocket-e2e/src/support/steps/lcp-beacon-script.ts:131:32)

It does not happen on trunk. Did you get this on your side as well or not?

Overall, while this is a good idea in the end to early fail in case we get an error from a WP CLI command, it seems this is too aggressive currently. I would suggest to:

Limit this PR to the first step we need: checking if a plugin activated is activated.
Open a new GH issue to enforce WP CLI response check at eery call, and then we will investigate how to achieve this.

I see in your PR you defined checkPluginStatus but never use it. Maybe just focus on that to begin with?

Khadreal commented 3 months ago

Okay, I'll modify this. It's related if an error is thrown the steps will fail

jeawhanlee commented 3 months ago

Pending: Run the test to be sure it works as expected.

jeawhanlee commented 3 months ago

Thanks for the PR @Khadreal

This works as expected, an error is thrown when plugin could not be activated and runs as expected when plugin is activated.
However, I can still see the first test failing like @MathieuLamiot stated earlier, which is not the case on trunk.

Khadreal commented 3 months ago

An error is thrown when plugin could not be activated and runs as expected when plugin is activated.

Isn't that what was expected? we should bail early when a plugin is not activated or am I missing something ? cc @jeawhanlee

jeawhanlee commented 3 months ago

@Khadreal Sorry if my first point wasn't clear, The first point works as expected, however it's the last we should consider.

Khadreal commented 3 months ago

@Khadreal Sorry if my first point wasn't clear, The first point works as expected, however it's the last we should consider.

Please try again, issue resolved. Updated the branch to what we have on trunk.

wp-media / wp-rocket-e2e

:closes: Add error check for wp command #109

Description

Documentation

User documentation

Technical documentation

Type of change

New dependencies

Risks

Checklists

Feature validation

Documentation

Code style

Observability