Move TNumberProperty validValues to TProperty

[samreid]

No description provided.

[samreid]

Also, I noticed that units and range are incorrectly in TProperty. I'll take a look, but tagging @zepumph so he's aware I'm working on it.

[samreid]

I wrote a rough draft of this and to test it I went to add qunit tests, but ran into trouble because we cannot currently include TNumber in axon unit tests. Not sure if I should change axon unit tests to require phet-io (would make it difficult for non-phetio users to test axon) or move the unit tests to an inferior location.

[samreid]

Adding a dependency from axon unit => phet-io would also introduce a dependency from scenery unit tests on phet-io. @jonathanolson is this acceptable or should it be avoided?

[samreid]

I thought I could add shims and what not to get things working (by overriding tandem.addInstance and creating a fake TType), but ifphetio requires window.phet.chipper.brand = 'phet-io'; and that requires phet-io-query-parameters.js.

Anyways, here is a unit test I decided not to commit that exercises and tests TProperty.toStateObject:

  var TType = function() {};
  TType.toStateObject = function( instance ) {
    return instance;
  };
  TType.fromStateObject = function( stateObject ) {
    return stateObject;
  };
  TType.typeName = 'TType';

  test( 'Test TProperty toStateObject/fromStateObject', function() {

    var tandem = new phet.tandem.Tandem( 'test' );

    tandem.addInstance = function( instance, type, objectk ) {
      var stateObject = type.toStateObject( instance );
      equal( stateObject.value, 0, 'toStateObject should match' );
      debugger;
    };
    var property = new phet.axon.Property( 0, {
      phetioValueType: TType,
      tandem: tandem,
      validValues: [ 0, 1, 2, 3 ]
    } );
  } );

[samreid]

@zepumph can you look at the preceding commit? 32ede1d

[zepumph]

@samreid that commit looks very good. How should we proceed about phet-io in unit tests?

[zepumph]

There was one problem addressed in https://github.com/phetsims/phet-io/issues/1246

[samreid]

It seems like a question for the development team about how to run phet-io specific tests for common code repos like axon/dot/kite/scenery. Should we have two test harnesses for each repo, one with phet-io and one without? Or since our team is the only one running unit tests at the moment, just require phet-io for the unit tests? The latter would be simple, but unit tests would crash for anyone who couldn't checkout the private phet-io repo.

[pixelzoom]

@samreid said:

It seems like a question for the development team about how to run phet-io specific tests code repos like axon/dot/kite/scenery

The question is more general - it applies to phet-io unit tests for all repositories.

All unit tests (phet-io and others) should be run automatically on bayes. Unit tests should be runnable from build tools, so that we can easily perform relevant unit tests before commits/builds/etc. And unit tests should be run automatically as part of an RC or production build.

Should we have two test harnesses for each repo, one with phet-io and one without? Or since our team is the only one running unit tests at the moment, just require phet-io for the unit tests? The latter would be simple, but unit tests would crash for anyone who couldn't checkout the private phet-io repo.

I don't have a preference as to whether phet-io unit tests should be kept separate from other unit tests. That seems like a management decision, based on relative cost of separate-vs-integrated, and whether unit tests (and tools that use those tests) need to be runnable by third parties.

[pixelzoom]

... and whether unit tests (and tools that use those tests) need to be runnable by third parties.

PhET Development Overview contains instructions about how to build sims, and there have been related inquires on Google Group. So third parties have been using build tools.

[pixelzoom]

11/9/17 dev meeting notes:
• make build tools usable by 3rd parties
• conditionally execute phet-io qunit tests if required dependencies are available
• investigate what to do on bayes

[zepumph]

Overall developers in today's meeting don't feel like many people use our build tools/ support libraries outside of phet.

That being said it is a big step to force phet-io into all of those repos. For now we are going to try to conditionally add phet-io tests to the same qunit test for a repo like axon. If you try to load the phet-io script but get a 404 (since our private repos aren't checked out) then it would skip the phet-io unit tests.

We were also thinking that then it would be good for bayes to be adapted to run in another testing environment such does all the same tests, but without any private repos checked out. This would be a good step to take to make sure that our open source project remains usable by outside devs

Consensus from @ariel-phet:

Implement conditional phet-io unit tests, then lets touch base about adapting bayes to test without private repos checked out.

[samreid]

I refined the unit test to this point:

  var TType = function() {};
  TType.toStateObject = function( instance ) {
    return instance;
  };
  TType.fromStateObject = function( stateObject ) {
    return stateObject;
  };
  TType.typeName = 'TType';

  test( 'Test TProperty toStateObject/fromStateObject', function() {

    var tandem = {};

    tandem.addInstance = function( instance, type, object ) {
      debugger;
      var stateObject = type.toStateObject( instance );
      equal( stateObject.value, 0, 'toStateObject should match' );
    };
    var property = new axon.Property( 0, {
      phetioValueType: TType,
      tandem: tandem,
      validValues: [ 0, 1, 2, 3 ]
    } );
  } );

But it's still not working because the ifphetio! plugin is disabled.

[samreid]

Still hesitant to include initialize-globals.js as a dependency for axon unit tests....

[zepumph]

Rather than adding initialize-globals to unit tests, could we include a unit test workaround in the ifphetio plugin?

I see

        var brand = window.phet && window.phet.chipper && window.phet.chipper.brand;
        if ( brand === 'phet-io' ) {
          require( [ id ], load );
        }
        else {
      /* no op function */
       }

But what if we added to the condition like window.phet.isUnitTest? Or something more robust so that we could use it where ever we needed access to the phet-io code without the full stack of the sim.

[pixelzoom]

@zepumph wrote:
if ( brand === 'phet-io' ) {

Testing brand to determine whether to do something related to PhET-iO is an anti-pattern. The 'phet-io' brand is currently the only brand that has PhET-iO features. That will not necessarily be true in the future; other brands may have phet-io feature and should run phet-io unit tests.

[samreid]

I created a new issue in the PhET-iO repo focused on how to test common code repo PhET-iO things. This issue will remain focused on "Move TNumberProperty validValues to TProperty" and on hold until we can add tests for it.

[samreid]

Added unit test in preceding commit, closing.