Prevent re-creating session file on end

[blueo]

saveState will re-create a session file after the test session has ended. This adds a condition to check if the test session has been cleared before writing the file.

[dnsl48]

feels like it might be better to perform that kind of checks outside this method so we keep it simple and its semantics reflect its name and concern

[blueo]

feels like it might be better to perform that kind of checks outside this method so we keep it simple and its semantics reflect its name and concern

Thanks @dnsl48, yeah I wasn't totally happy with this. What do you think of splitting the saveState out from applyState. So making applyState just return the newly modified state which is then passed to saveState for persistence. Would add another call to everywhere applyState is currently used but would allow checks before saving and might just be a bit cleaner.

[dnsl48]

What do you think of splitting the saveState out from applyState

Sounds like a plan. I guess we don't really have a choice right? :)

[dnsl48]

making applyState just return the newly modified state

it also feels weird that applyState actually modifies the state... Since you're looking into it anyway, could you please consider if we might refactor this too?

[blueo]

making applyState just return the newly modified state

it also feels weird that applyState actually modifies the state... Since you're looking into it anyway, could you please consider if we might refactor this too?

I was thinkingapplyState will return a new copy of the state - not touching the 'original' state - is that what you mean?

[dnsl48]

I was thinkingapplyState will return a new copy of the state

well, I was rather thinking about moving out of it any modifications performed on the state, so it would only connect to the database preliminarily created outside. So it would only "apply" and not actually "modify" $state anyhow

[chillu]

@blueo Do you agree with Serge, and can implement his suggestion?

[blueo]

Hey @chillu i was looking into it but haven't quite managed to get all state change out. the biggest hurdle being that the test db name may change. But in general yes I agree and I'll be working on re-factoring it the next chance I get :)

[blueo]

Hi @dnsl48 :) So that turned into a bit of a rabbit hole. I've refactored to separate our applyState and saveState. In the process I've actually addressed the other two PRs I have open #64 and #63 (as they'd need significant change also).

given it is quite a big change I've added some unit tests to try cover some basic scenarios - i don't think this is exaustive atm. I'm using this refactor on an internal project so will be able to beef up the tests a bit as I go. cc @chillu

[dnsl48]

wow, that definitely looks cool! I'll have a play with it some time this week.

[dnsl48]

Hey @blueo, sorry for delay. I've run it, but it breaks otherwise green tests on my local setup.
That's what I'm getting:

./vendor/bin/behat @framework                                                                                                             
Bootstrapping
Creating test session environment                                                                                                                                             
Temp Database: ss_tmpdb_1557720566_5399227
                                                                                       
@retry                                                                                 
Feature: Log in
  As an site owner               
  I want to access to the CMS to be secure
  So that only my team can make content changes
                                           
  Scenario: Bad login                                                 # vendor/silverstripe/admin/tests/behat/features/login.feature:7
    Given I log in with "[email protected]" and "badpassword"           # SilverStripe\BehatExtension\Context\LoginContext::stepILogInWith()
    │                                     
    ╳  Notice: Trying to get property of non-object in vendor/silverstripe/testsession/src/TestSessionEnvironment.php line 648
    │
    └─ @AfterStep # SilverStripe\Framework\Tests\Behaviour\FeatureContext::waitResponsesAfterStep()                                   
    Then I should see "The provided details don't seem to be correct" # SilverStripe\Framework\Tests\Behaviour\FeatureContext::assertPageContainsText()

I'll do some debugging later this week, but it seems like TestSessionState isn't being persisted properly anymore.

[blueo]

ah thanks @dnsl48 I'll have a look, i should probably add some basic behat checks

[blueo]

Hey there, @dnsl48 yep, I missed creating the testsession. I also had a broken mailer setup, I've addressed both and added some test cases for it 👍 framework behat now runs green for me

[dnsl48]

Nice! Do you mind resolving the conflict or rebasing the branch yet?

[blueo]

Nice! Do you mind resolving the conflict or rebasing the branch yet?

done :)

[dnsl48]

I know that's not changed in the PR, but could we change it to
!is_subclass_of($mailer, Mailer::class)?

silverstripe-testsession/src/TestSessionEnvironment.php

Line 358 in b4a54a5

if (!class_exists($mailer) || !is_subclass_of($mailer, 'SilverStripe\\Control\\Email\\Mailer')) {

[dnsl48]

That's really good job!
Thank you for doing those tough refactoring bits and untying this knot!

Let's think if we could spare backwards compatibility or otherwise we could think on moving this changes to master (and potentially the next major version of the module).

[dnsl48]

Here's the fix for TestMailer - silverstripe/silverstripe-behat-extension#188
It should be merged after this PR gets merged.

[dnsl48]

good work 👍

P.S.
Tested locally on ss/installer (@framework and @cms). Though, also requires silverstripe/silverstripe-behat-extension#188

[dnsl48]

Good to merge when silverstripe/silverstripe-behat-extension#188 is merged

[blueo]

@dnsl48 it feels like we might be able to merge/tag this before silverstripe/silverstripe-behat-extension#188 ? I'd be interested in doing a little more work on this module around assets etc but I'm worried this will become the never-ending-pr if I do...

[dnsl48]

feels like we might be able to merge/tag this before silverstripe/silverstripe-behat-extension#188 ?

Sorry, no can do, without 188 it breaks email related tests. On the other hand, I'm quite certain this PR will be merged "as is", so if you base your other PRs on top of this one, you most likely won't even need to rebase

[blueo]

but if this is a new major version - you don't need to use it until you want right? So the email related tests would still use the older version?

[dnsl48]

That's right. I think you're right. We can deal with behat-extension separately.

[blueo]

cool thanks!