Currently, a number of features and behaviors are AMP-specific and rely on logic implemented in the Context class for determining the AMP context and mode.
In PHP tests, this has been less than ideal to handle and requires us mocking a dummy object with stubbed responses to is_amp method calls. Because classes under test still need to be instantiated with a real Context instance, we need to do that first and then force-set the private context property with the dummy instance. This is problematic because any internally instantiated classes will have the Context instance that was passed in the constructor rather than the fake one.
This is all overly complicated simply because Context is defined as final which it has been since the initial release and prevents us from using a simple sub-class for use in tests. This is unnecessarily restrictive as nearly our entire PHP codebase is intended to be private so anyone who might be extending this class would be "doing it wrong" so we shouldn't need to protect developers from taking risks that they shouldn't.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
The \Google\Site_Kit\Context class should no longer be declared as final
Tests requiring AMP-specific context should use an instance of Context which is fixed to return AMP-specific values for relevant methods
::is_amp() should always return true unconditionally
::get_amp_mode() should always return primary or secondary using class constants on the Context class
AMP mode-specific instances should be used rather than generic/configurable instances
All existing workarounds for faking AMP context in PHPUnit tests should be updated to use the new instances
Implementation Brief
Merge #2125
Test Coverage
This issue is about refactoring for tests; no additional coverage is necessary
Visual Regression Changes
N/A
QA Brief
PHPUnit tests should all pass
There should be no instances of a mocked class of MockClass used in place of Context in tests
@felixarntz I've updated the ACs here to be less implementation-specific and accommodate the changes we agreed on in #2125 while preserving the original purpose of the issue.
Feature Description
Currently, a number of features and behaviors are AMP-specific and rely on logic implemented in the
Context
class for determining the AMP context and mode.In PHP tests, this has been less than ideal to handle and requires us mocking a dummy object with stubbed responses to
is_amp
method calls. Because classes under test still need to be instantiated with a realContext
instance, we need to do that first and then force-set the privatecontext
property with the dummy instance. This is problematic because any internally instantiated classes will have theContext
instance that was passed in the constructor rather than the fake one.This is all overly complicated simply because
Context
is defined asfinal
which it has been since the initial release and prevents us from using a simple sub-class for use in tests. This is unnecessarily restrictive as nearly our entire PHP codebase is intended to be private so anyone who might be extending this class would be "doing it wrong" so we shouldn't need to protect developers from taking risks that they shouldn't.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
\Google\Site_Kit\Context
class should no longer be declared asfinal
Context
which is fixed to return AMP-specific values for relevant methods::is_amp()
should always returntrue
unconditionally::get_amp_mode()
should always returnprimary
orsecondary
using class constants on theContext
classImplementation Brief
Test Coverage
Visual Regression Changes
QA Brief
MockClass
used in place ofContext
in testsChangelog entry