Skip to content

Conversation

@vaishaliagola27
Copy link

Add test case for AMP_AdManager class

Covers point 3 in #60 (comment)

Copy link
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

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

phpcs scanning turned up:

🚫 17 errors

⚠️ 1 warning


hashes-api-scanning skipped

Posting will continue in further review(s)

/**
* This function sets the instance for class \AMP_AdManager\Shortcode.
*/
public function setUp(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 Error: void return type is not present in PHP version 7.0 or earlier (PHPCompatibility.FunctionDeclarations.NewReturnTypeDeclarations.voidFound).

Copy link
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

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

Previous scan continued.

*
* @var AMP_AdManager
*/
protected $_instance;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Warning: Property name "$_instance" should not be prefixed with an underscore to indicate visibility (PSR2.Classes.PropertyDeclaration.Underscore).

call_user_func(
sprintf( 'has_%s', $hook['type'] ),
$hook['name'],
array(
Copy link
Member

Choose a reason for hiding this comment

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

Use [] array syntax here

* @param array $args WP query arguments.
* @param array $conditions wp query conditions.
*/
public function mock_wp_query( $args, $conditions ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this method to Utility class so it can be reused wherever needed

Copy link
Author

Choose a reason for hiding this comment

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

sure


// Update settings after updating option.
AMP_AdManager::$amp_settings = get_option( 'amp-admanager-menu-settings' );
$output = Utility::buffer_and_return( array( $this->_instance, 'load_amp_resources' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Use [] array syntax

) );
update_option(
'amp-admanager-menu-settings',
array(
Copy link
Member

Choose a reason for hiding this comment

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

use [] syntax here

$user_mock = $this->factory->user->create_and_get( [ 'role' => 'administrator' ] );
wp_set_current_user( $user_mock->ID );
$_GET['amp_validate'] = true;
$output_user = Utility::buffer_and_return( array( $this->_instance, 'load_amp_resources' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Use [] array syntax

@deepaklalwani97
Copy link
Member

@vaishaliagola27 The AMP admanager class has some missing code coverage can you please fix it or ignore it with valid comments if it cannot be covered.

Copy link
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

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

phpcs scanning turned up:

🚫 3 errors


hashes-api-scanning skipped

Copy link
Member

@deepaklalwani97 deepaklalwani97 left a comment

Choose a reason for hiding this comment

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

LGTM

@vaishaliagola27 vaishaliagola27 merged commit 4215d94 into dev-unit-test Feb 7, 2020
@vaishaliagola27 vaishaliagola27 deleted the dev-unit-test-ampadmanager branch February 7, 2020 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants