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.

Sign up for GitHub

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

Jump to bottom

Fix broken codeception tests #201

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

Fix broken codeception tests #201

wants to merge 1 commit into from

Conversation

deathbird
Copy link
Contributor

Problem and/or solution

The previous commit introduced some changes that caused codeception
unit tests to break. This commit fixes this by introducing a new
service class Transifex_Live_Integration_WP_Services to wrap WP API
functionality. This refactoring enables mocking the WP API in the
tests.
Also a new test class, BaseTestCase, is introduced to cater for
different PHPUnit versions between production and development
environment.

How to test

  • Run unit tests locally by first downloading codeception in the plugin directory
    and then executing:
export TEST_ENV='dev'
php codecept.phar run unit --debug
  • Manual testing of generated urls:
    • From WP admin UI in a sample post add a block of HTML content that contains links that point to both local or external links.
    • From WP admin UI create a new (Appearance > Menus) that contains both local and external links. Add the menu to some WP pages.
    • Preview the above created links in the frontend selecting a translated language from the language selector.

Important note: The plugin does NOT support URL rewritting of partial
links (without a host part eg: '/test-page2')

Diontsoumas
Diontsoumas suggested changes Apr 2, 2019
View reviewed changes
includes/lib/transifex-live-integration-rewrite.php Outdated
@@ -40,6 +40,7 @@ public function __construct( $settings, $rewrite_options ) {
Plugin_Debug::logTrace();
include_once TRANSIFEX_LIVE_INTEGRATION_DIRECTORY_BASE . '/includes/common/transifex-live-integration-validators.php';
include_once TRANSIFEX_LIVE_INTEGRATION_DIRECTORY_BASE . '/includes/override/transifex-live-integration-generate-rewrite-rules.php';
include_once __DIR__ .'/transifex-live-integration-wp-services.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not TRANSIFEX_LIVE_INTEGRATION_DIRECTORY_BASE as above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mystery to me how when this class is included in tests it resolves TRANSIFEX_LIVE_INTEGRATION_DIRECTORY_BASE constant to its correct value :) . That's why I referenced the added include relatively via path. I agree with you that for uniformity reasons though it should use TRANSIFEX_LIVE_INTEGRATION_DIRECTORY_BASE as above.

includes/lib/transifex-live-integration-wp-services.php Outdated

/*
* Wraps WP site_url().
* @return string Site URL link with optional path appended.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the optional path appended part in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the docstring from the site_url() source code and this created the misunderstanding: https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/link-template.php#L3141. For this I'll remove the @return line altogether.

includes/lib/transifex-live-integration-wp-services.php Outdated
<?php

/*
* Language rewrites
Copy link
Contributor

Choose a reason for hiding this comment

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

Language rewrites doesn't seem accurate for this class. Maybe add a couple of lines of what should be added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it's not accurate, will remove. No need for extra description since we follow the convention 1 class per file and the class contains docstrings.

tests/unit/BaseTestCase.php Outdated
* export TEST_ENV=<environment>
* php codecept.phar run unit --debug
*/
$test_env = null !== getenv('TEST_ENV') ? getenv('TEST_ENV') : 'prod';
Copy link
Contributor

Choose a reason for hiding this comment

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

$test_env is probably not a good attribute name, especially if it can have the prod value. What about just $env?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, prod is the CI environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually 'prod' is just a placeholder that stands for: 'NOT development environment'. AFAIK the CI environment does not have a distinction of environment.

tests/unit/BaseTestCase.php Outdated
$test_env = null !== getenv('TEST_ENV') ? getenv('TEST_ENV') : 'prod';

if ($test_env !== 'dev') {
// php 5.6, production environment
Copy link
Contributor

Choose a reason for hiding this comment

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

By production environment I guess you mean the environment in the CI. Can you please rename it here?

tests/unit/BaseTestCase.php
class BaseTestCase extends \PHPUnit_Framework_TestCase {}
} else {
// php 7+, dev environment
class BaseTestCase extends PHPunit\Framework\TestCase {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that PHP 7+ is used for developing, which may not be always the case (for example if an engineer uses another docker-compose file with an older PHP version). I wonder if we can try / catch the error if the first TestCase class doesn't exist, then load the alternative one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bravo @Diontsoumas, excellent idea! In fact PHP has the function class_exists() just for that ;)

tests/unit/ReverseHardLinkTest.php Outdated
$result = Transifex_Live_Integration_Rewrite::reverse_hard_link(
// stub
$rewrite->wp_services = \Codeception\Stub::make(Transifex_Live_Integration_WP_Services::class, ['get_site_url' => $i['host']]);
$result = $rewrite->reverse_hard_link(
$i['lang'], $i['link'], $i['languages_map'], $i['souce_lang'], $i['pattern']
);

// eval(\Psy\sh());
Copy link
Contributor

Choose a reason for hiding this comment

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

Those two lines have comments, can we please delete them?

@deathbird
Fix broken codeception tests
f895b64 
The previous commit introduced some changes that caused codeception
unit tests to break. This commit fixes this by introducing a new
service class Transifex_Live_Integration_WP_Services to wrap WP API
functionality. This refactoring enables mocking the WP API in the
tests.
Also a new test class, BaseTestCase, is introduced to cater for
different PHPUnit versions between production and development
environment.
@ThemisB ThemisB mentioned this pull request Jun 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Diontsoumas Diontsoumas Awaiting requested review from Diontsoumas

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@deathbird @Diontsoumas