Closed blueo closed 5 years ago
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.
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? :)
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?
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?
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
@blueo Do you agree with Serge, and can implement his suggestion?
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 :)
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 https://github.com/silverstripe/silverstripe-testsession/pull/64 and https://github.com/silverstripe/silverstripe-testsession/pull/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
wow, that definitely looks cool! I'll have a play with it some time this week.
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 "bad@example.com" 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.
ah thanks @dnsl48 I'll have a look, i should probably add some basic behat checks
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
Nice! Do you mind resolving the conflict or rebasing the branch yet?
Nice! Do you mind resolving the conflict or rebasing the branch yet?
done :)
I know that's not changed in the PR, but could we change it to
!is_subclass_of($mailer, Mailer::class)
?
https://github.com/silverstripe/silverstripe-testsession/blob/b4a54a58696c72999a66ffec6fb8020a8a35c7c7/src/TestSessionEnvironment.php#L358
Here's the fix for TestMailer - https://github.com/silverstripe/silverstripe-behat-extension/pull/188 It should be merged after this PR gets merged.
Good to merge when https://github.com/silverstripe/silverstripe-behat-extension/pull/188 is merged
@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...
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
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?
That's right. I think you're right. We can deal with behat-extension separately.
cool thanks!
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.