getStateFragment blindly fetches from the key/value store

[walterdolce]

Hi, I noticed the getStateFragment method in ScenarioState blindly fetches what gets requested from it without checking whether the key actually exists.

# ScenarioState.php
    /**
     * {@inheritdoc}
     */
    public function getStateFragment($key)
    {
        return $this->store[$key];
    }

I am aware the class could be extended to match the desired behaviour (raise an exception if the key is requested and does not exist? return null? etc), I was just wondering whether providing a "basic" ScenarioState implementation has been intentional or not.

• If it was intentional, then I think it would be good to explicitly state why the component has been designed the way it's been designed and to encourage other people like me to extend it to match the specific requirements one might have.
• If it was not intentional, I am happy to contribute and improve this component. Though this would beg questions like:
  -- Should the key fetching behaviour be configurable by users? I.e. to have something like raise_exception_when_state_is_missing: true in the config.
  -- Should it return null?
  -- Should it error, given some state has been requested but it's not there?

Let's discuss :)
Thanks!

[gorghoa]

I think I was relying on how the user has configured its php to handle this case ^^.

I can understand that this may very well be not enough, especially in the context of testing: when the goal is high quality code and explicit fast failure.

My position would be to raise an exception (StateFragmentNotFoundException?), I don’t think we should add some config for this matter. The test cannot run without its dependency, end of story.

It would be a strong enforcement to write consistent tests, without relying on underlying if/else or try/catch or even by prefixing with an @ to catch the notice error 😃. As a result, tests should remain simple.

What do you think?

[walterdolce]

[...] The test cannot run without its dependency, end of story. [...] What do you think?

Completely agree.

For new unit tests, would you expect contributors to keep using PHPUnit or would you be open to using PhpSpec as well?

[vincentchalamon]

@walterdolce As unit tests are written in PHPUnit, you should keep using it.

[walterdolce]

Sounds good to me. PR raised.

[gorghoa]

Fixed by #21.

Thanks @walterdolce and @vincentchalamon for your help :)