peridot icon indicating copy to clipboard operation
peridot copied to clipboard

Run-time scope properties leak across suites

Open ezzatron opened this issue 9 years ago • 6 comments

Just noticed this issue, not sure if it's by design, but it seems like a potential source of frustration when trying to debug a test.

To reproduce:

a.spec.php:

<?php

describe('a', function () {
    it('leaks scope variables across suites', function () {
        $this->tmp = 'a';
    });
});

b.spec.php:

<?php

describe('b', function () {
    it('leaks scope variables across suites', function () {
        expect($this->tmp)->to->equal(null);
    });
});

Run these two suites, ensuring that a runs before b (which may be file system dependent order, not sure), and you'll get this output:

$ peridot /path/to/tests                                                                                                                                                                                     (master✱) [13:54]

  a
    ✓ leaks scope variables across suites

  b
    1) leaks scope variables across suites


  1 passing (1 ms)
  1 failing

  1) b leaks scope variables across suites:
     Expected null to be identical to "a"
      #0 /path/to/peridot-php/leo/src/Assertion.php(224): Peridot\Leo\Responder\ExceptionResponder->respond(Object(Peridot\Leo\Matcher\Match), Object(Peridot\Leo\Matcher\Template\ArrayTemplate), 'Expected null t...')
      (snip)

Also note that I'm fairly sure Leo's assertion message is back-to-front, but that's a separate issue.

ezzatron avatar Feb 23 '16 04:02 ezzatron

@ezzatron this was always kind of the looming fear for me with scopes. It has to do with the top level suite that everything is implicitly nested under. The 2.0 branch I started was working towards something more robust, but I'm still not settled on a good solution. Scopes sort of try to recreate an inheritance system without the dreaded extends keyword - tricky business.

brianium avatar Feb 24 '16 01:02 brianium

Yeah, I can imagine it's tricky. FWIW, this is not causing any real issue for me right now. At least there's an issue number to refer back to for future updates :)

ezzatron avatar Feb 24 '16 02:02 ezzatron

I actually ran across this as well. We started running into memory issues, which look like they might be from so many objects piling up in the scope. There are other ways to deal with it if that is the case (first is to refactor our tests), but I thought I'd throw in my 2cents.

drake-p avatar Mar 07 '16 17:03 drake-p

@brianium

describe('Some spec', function () {
    $this->value = 'initial value';

    context('Inner context 1', function () {
       $this->value = 'overwritten';

       it('should use overwritten value', function() {
          expect($this->value)->to->be->equal('overwritten');
       });
    });

    it('should use initial value', function () {
        expect($this->value)->to->be->equal('initial value');
    });

});

I looked through source code and I have a couple of questions:

  • Why is scope implementation moved to separate package?
  • Why is peridot use approach with root scope on top and multiple nested scopes an low level? Would it be just to make single root scope and everything inherited from it? I don't see any benefits of current implementation event from plugins point of view.

I really like this projects (since this is only one usable jasmine-like testing framework) and maybe I could help you somehow? for example reimplement scope system fully?

fesor avatar Jun 26 '16 12:06 fesor

@fesor - Scope being moved to a separate package was a mistake in hindsight. The work that was started on 2.0 worked to bring it back into the main repo.

Scope are definitely flaws a bit. They attempted to offer a mixin style of inheritance at runtime so plugins could expose functions to tests.

It is definitely in need of improvement, so a rewrite might go a long way. I can't remember the justification for having a root scope property at the top. Since a scope is effectively the state of a spec, I think it was so nested specs would have access to that outer state.

Scopes became bags for state AND functionality which kind of confused things a bit. I always wanted to improve how scope methods were resolved. Seems a little backwards to look at child scopes when resolving functions.

Was any of that helpful? Thanks for your kind words :) - I am hoping to talk to @ezzatron shortly as I am currently looking for maintainers of the Peridot org. I have not had much time to develop Peridot lately, and don't get to use it much in my current projects

brianium avatar Jun 26 '16 19:06 brianium

@brianium thanks for answers. I had a pet project which adds jasmine/rspec like specs to phpspec and I used something like this to make scopes work:

https://gist.github.com/fesor/09c91090cef3da0da0e143cca3ae2d62

rootScope is outside of spec and it can be used to add global things. This is pretty much like angular's scope system works.

p.s. the scopes problem currently the only blocker for me to start use it in my team.

fesor avatar Jun 27 '16 08:06 fesor