-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
Sporadic failures in silverstripe/admin CI #1671
Comments
Anecdotally, the |
I've noticed Edit: I'm trying |
For the js failures, when one fails, download the artifacts and try |
Created an issue to do this is CI silverstripe/gha-ci#104 |
I had a good go at getting behat to re-run a scenario on failure, I just can't do it though without resorting to some major hacks. There's just doesn't seem to be enough exposed by behat hooks to allow me to do this. This will get it to re-run in a 'good enough' manner in RuntimeFeatureTester.php public function test(Environment $env, $spec, $skip = false)
{
$results = array();
foreach ($spec->getScenarios() as $scenario) {
$tester = $scenario instanceof OutlineNode ? $this->outlineTester : $this->scenarioTester;
$setup = $tester->setUp($env, $spec, $scenario, $skip);
$localSkip = !$setup->isSuccessful() || $skip;
$testResult = $tester->test($env, $spec, $scenario, $localSkip);
$teardown = $tester->tearDown($env, $spec, $scenario, $localSkip, $testResult);
// rerun here
if (!$testResult->isPassed()) {
$setup = $tester->setUp($env, $spec, $scenario, $skip);
$localSkip = !$setup->isSuccessful() || $skip;
$testResult = $tester->test($env, $spec, $scenario, $localSkip);
$teardown = $tester->tearDown($env, $spec, $scenario, $localSkip, $testResult);
}
$integerResult = new IntegerTestResult($testResult->getResultCode());
$results[] = new TestWithSetupResult($setup, $integerResult, $teardown);
}
return new TestResults($results);
} This is obviously directly modifying upstream code. Which we could do in CI in a very hacky way i.e. just in our own version of the file ... or cleaner just fork behat entirely and use our own fork and just never receive updates again.. You CAN use an /** @AfterScenario */
public function after(AfterScenarioScope $scope)
{
$env = $scope->getEnvironment();
$spec = $scope->getFeature();
$scenario = $scope->getScenario();
$testResult = $scope->getTestResult();
// $tester = 'nope'; :-(
} BUT there's just no way to get the I even tried bootstrapping a new Application and using reflection nonsense though I wasn't able to access it. I'd rather not fork behat, if we really want retry perhaps the just copying in our own file in CI could work if we took an md5 signature of the file to be replaced and validate that it hasn't been updated upstream first, and if it has just |
@GuySartorelli mentioned that there's a way to access the service container during bootstrapping |
See this PR for an example. The subclass would have the code from Steve's example above, or something similar - which would then mean we're rerunning all failed scenarios for any failure regardless of what module is being rerun. I think it should be possible to get a boolean from the behat.yml into the extension for the purposes of setting this to happen only in CI. |
Probably, we can just enhance those steps(function) that usually fail. There is some example how we can do this. |
The suggestions in that link are essentially what the |
Had another good go at this, still no luck While I can get access to the service container from the afterScenario hook, the tester isn't normally actually available then. I tried extracting the tester out instead of the container early on, however I was unable to use it later, it threw an error. I think at this point we need to either fork behat or copy in patch file in CI (my preference) // behat-extensions/src/Extension.php
// ...
public function process(ContainerBuilder $container)
{
// ...
// Set the tester in the tester holder singleton
// This needs to happen in process() rather than load() because Injector isn't available in load()
// Note that we put the tester into the holder rather than the entire container because the services
// in the container actually greater between this point and the BasicContainer::after() hook, and the
// tester isn't available at that point
$tester = $container->get(TesterExtension::SCENARIO_TESTER_WRAPPER_TAG . '.isolating');
$testHolder = Injector::inst()->get(TesterHolder::class, true);
$testHolder->setTester($tester);
} // behat-extension/src/TesterHolder.php
<?php
namespace SilverStripe\BehatExtension;
use Behat\Behat\Tester\Runtime\IsolatingScenarioTester;
/**
* Class intended to be used as a singleton to hold the IsolatingScenarioTester instance
* which is accessible during the test run, so that it can be accessed from withing
* BasicContext after the scenario has been run
*/
class TesterHolder
{
private IsolatingScenarioTester $tester;
public function getTester(): IsolatingScenarioTester
{
return $this->tester;
}
public function setTester(IsolatingScenarioTester $tester): void
{
$this->tester = $tester;
}
} // behat-extensions/src/Context/BasicContext
// ...
/** @AfterScenario */
public function after(AfterScenarioScope $scope)
{
$skip = false;
$env = $scope->getEnvironment();
$spec = $scope->getFeature();
$scenario = $scope->getScenario();
$testResult = $scope->getTestResult();
$testerHolder = Injector::inst()->get(TesterHolder::class, true);
$tester = $testerHolder->getTester();
// re-run test if it failed
// TODO: only do this in CI .. test custom config variable?
if (!$testResult->isPassed()) {
// This code is copied from RuntimeFeatureTester::test()
$setup = $tester->setUp($env, $spec, $scenario, $skip);
$localSkip = !$setup->isSuccessful() || $skip;
$testResult = $tester->test($env, $spec, $scenario, $skip);
// $teardown = $tester->tearDown($env, $spec, $scenario, $localSkip, $testResult);
}
} Results in
|
We have a few options here that I can see. All of these are done within the Option 1: Decorate the scenario testerClick to see the `RetryScenarioTester` code<?php
namespace SilverStripe\BehatExtension\Tester;
use Behat\Behat\Tester\ScenarioTester;
use Behat\Gherkin\Node\FeatureNode;
use Behat\Gherkin\Node\ScenarioInterface;
use Behat\Testwork\Environment\Environment;
use Behat\Testwork\Tester\Result\TestResult;
class RetryScenarioTester implements ScenarioTester
{
private ScenarioTester $decoratedTester;
public function __construct(ScenarioTester $decoratedTester)
{
$this->decoratedTester = $decoratedTester;
}
public function setUp(Environment $env, FeatureNode $feature, ScenarioInterface $scenario, $skip)
{
return $this->decoratedTester->setUp($env, $feature, $scenario, $skip);
}
public function test(Environment $env, FeatureNode $feature, ScenarioInterface $scenario, $skip)
{
$result = $this->decoratedTester->test($env, $feature, $scenario, $skip);
if (!$skip && !$result->isPassed()) {
/* Some logic goes here to retry the scenario */
}
return $result;
}
public function tearDown(Environment $env, FeatureNode $feature, ScenarioInterface $scenario, $skip, TestResult $result)
{
return $this->decoratedTester->tearDown($env, $feature, $scenario, $skip, $result);
}
} <?php
use Behat\Behat\Tester\ServiceContainer\TesterExtension;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Behat\Testwork\ServiceContainer\Extension as ExtensionInterface;
use SilverStripe\BehatExtension\Tester\RetryScenarioTester;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;
class Extension implements ExtensionInterface
{
// ...
public function load(ContainerBuilder $container, array $config)
{
// ...
// Decorate the scenario tester with our own class so we can retry when they fail
$definition = new Definition(RetryScenarioTester::class, [
new Reference(TesterExtension::SCENARIO_TESTER_ID),
]);
$definition->addTag(TesterExtension::SCENARIO_TESTER_WRAPPER_TAG);
$container->setDefinition(TesterExtension::SCENARIO_TESTER_WRAPPER_TAG . '.silverstripe', $definition);
}
} Option 2: Replace the scenario tester with our own classUnfortunately <?php
use Behat\Behat\Tester\ServiceContainer\TesterExtension;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Behat\Testwork\ServiceContainer\Extension as ExtensionInterface;
use SilverStripe\BehatExtension\Tester\RetryScenarioTester;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;
class Extension implements ExtensionInterface
{
// ...
public function load(ContainerBuilder $container, array $config)
{
// ...
// Replace the scenario tester with our own class so we can retry when they fail
$definition = new Definition(RetryScenarioTester::class, array(
new Reference('tester.step_container'),
new Reference(TesterExtension::BACKGROUND_TESTER_ID)
));
$container->setDefinition(TesterExtension::SCENARIO_TESTER_ID, $definition);
}
} Option 3: Replace the feature tester with our own classUnfortunately <?php
use Behat\Behat\Tester\ServiceContainer\TesterExtension;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Behat\Testwork\Environment\ServiceContainer\EnvironmentExtension;
use Behat\Testwork\ServiceContainer\Extension as ExtensionInterface;
use SilverStripe\BehatExtension\Tester\RetryFeatureTester;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;
class Extension implements ExtensionInterface
{
// ...
public function load(ContainerBuilder $container, array $config)
{
// ...
// Replace the feature tester with our own class so we can retry when they fail
$definition = new Definition(RetryFeatureTester::class, [
new Reference(TesterExtension::SCENARIO_TESTER_ID),
new Reference(TesterExtension::OUTLINE_TESTER_ID),
new Reference(EnvironmentExtension::MANAGER_ID)
]);
$container->setDefinition(TesterExtension::SPECIFICATION_TESTER_ID, $definition);
}
} It is also possible to decorate the feature tester, but from the quick look I took we don't actually have enough information to know which scenario we need to retry if we do that. DisclaimerI was exploring what options we have for using dependency injection to avoid hot-patching in CI. I didn't explore what needs to happen to actually retry scenarios. |
Good thinking! I totally forgot that yes, when there's dependency injection available that's the correct way to add/override code :-) I've added some PRs to implement this |
First PR merged, reassigning to Steve for next steps |
Second PR merged, reassigning to Steve for next steps |
@GuySartorelli Have released new versions for behat-extension, good to merge the other 2 PRs now |
PRs merged. Reassigning to Steve to validate CI isn't broken |
Sporadic failures with behat + js job bundle.js in 4.13 here https://github.com/silverstripe/silverstripe-admin/actions/runs/7747592053
Caused an auto patch release for to fail #1670
Update: moving back into backlog to chat about with team - see my comment below for explanation
The retry PR's below are kind of not great, they just add
@retry
annotation which literally means "wait for up to 3 seconds when doing an assertion". What we really need is to completely retry failed scenarios.@Retry PRs
PRs - Make sure they are merged / released in this exact order!
The text was updated successfully, but these errors were encountered: