phetsims / tasks

General tasks for PhET that will be tracked on Github
MIT License
5 stars 2 forks source link

standardized unit tests #900

Open pixelzoom opened 7 years ago

pixelzoom commented 7 years ago

In https://github.com/phetsims/axon/issues/156#issuecomment-339147946 and https://github.com/phetsims/axon/issues/153#issuecomment-339158399, I had some unit tests that I wanted to add. But the axon unit tests didn't look like what I was expecting. So in both cases I punted by saying something like:

The above tests should be added to axon units tests but I was unsure how to add them.

In https://github.com/phetsims/axon/issues/156#issuecomment-340488450, @samreid said:

I should point out that axon is using a bit older version of qunit. We opted for the new one in query-string-machine and the test apis are a bit different (newer one is preferable).

I replied:

Thanks - I think that's what concerned me when I started looking at axon unit tests. I did add tests to query-string-machine, and axon didn't match what I was expecting.

Perhaps we should update all unit tests to use the same pattern? Is this something the QA might be interested in?

pixelzoom commented 7 years ago

In https://github.com/phetsims/axon/issues/153#issuecomment-340610458, we had problems with axon unit tests failing when run from scenery/kite/dot. In Slack, @jonathanolson discovered that axon unit tests use initialize-globals.js, which is supposed to only be run for simulations, and said "I'll fix up axon's unit test harness to be similar to the others."

pixelzoom commented 7 years ago

@jonathanolson noted in Slack: Axon unit tests are also loading a jQuery copy from a CDN, not sherpa's (I'll fix that too)

jonathanolson commented 7 years ago

Fixed up phet-core and axon's unit tests to be in line with dot/kite/scenery.

Perhaps we should update all unit tests to use the same pattern?

That sounds good to me, although it will require going through a LOT of unit tests. query-string-machine looks like it skipped ahead to a new version of qunit instead of using what the rest used at the time (and there are some decent reasons for doing so).

jonathanolson commented 7 years ago

There's also a lot of work that could be done cleaning up the unit test harnesses (since there's some duplicated HTML/JS for each place we have unit tests).

samreid commented 7 years ago

Is this something the QA might be interested in?

@pixelzoom can you clarify what you meant by this?

pixelzoom commented 7 years ago

I was wondering if QA might be able to convert unit tests from old to new pattern.

samreid commented 7 years ago

There's also a lot of work that could be done cleaning up the unit test harnesses (since there's some duplicated HTML/JS for each place we have unit tests).

What if we just had one test harness and the sims supplied only the JS files with the tests?

samreid commented 7 years ago

Also, even QueryStringMachine at qunit 2.0.1 is getting far behind the current version of qunit which is 2.4.1, here is the changelog: https://github.com/qunitjs/qunit/blob/2.4.1/History.md

samreid commented 7 years ago

Kite, dot and scenery are already somewhat composite tests. For instance, from kite:

  <script src="unit-tests.js"></script>
  <script src="../../../axon/tests/qunit/unit-tests.js"></script>
  <script src="../../../dot/tests/qunit/unit-tests.js"></script>
  <script src="../../../phet-core/tests/qunit/unit-tests.js"></script>

then

      runKiteTests( '.' );
      runAxonTests( '../../../axon/tests/qunit' );
      runDotTests( '../../../dot/tests/qunit' );
      runPhetCoreTests( '../../../phet-core/tests/qunit' );
samreid commented 7 years ago

We could also rework the unit tests for each repo to be focused just on that repo (not dependencies), then use an aggregator strategy like https://github.com/JamesMGreene/qunit-composite

samreid commented 7 years ago

I wonder if it would simplify things if we moved all our tests (even fuzz tests) to run as qunit tests.

jonathanolson commented 7 years ago

What if we just had one test harness and the sims supplied only the JS files with the tests?

It would also need to specify dependencies. For example in Scenery's unit tests (compared to Dot), it has a bunch of extra required dependencies from sherpa.

We could also rework the unit tests for each repo to be focused just on that repo (not dependencies), then use an aggregator strategy

Sounds nice.

I wonder if it would simplify things if we moved all our tests (even fuzz tests) to run as qunit tests.

Would require upgrading qunit then, as the version being used currently has a number of issues with continuous testing (as it can't properly report when all tests are complete).

samreid commented 7 years ago

@jonathanolson will discuss it with QA team.

samreid commented 7 years ago

My comments from https://github.com/phetsims/phet-io/issues/1261

It seems the right way to proceed is for each repo to provide a set of unit tests (whether different HTML files or different query parameters), and for us to rework how the unit tests are aggregated. We should eliminate duplication across the test harnesses and focus on making it easy to add unit tests, particularly to repos with no testing harness. We should update to the current version of QUnit and bring all tests up to that level. We should focus on factoring out the harness code, either by moving it to JS or by auto-generating it from a grunt task. We should see if https://github.com/JamesMGreene/qunit-composite can make a "composite of composites", then each sim can have a single entry point for all unit tests, then aqua can just point to the composites.

When we last discussed this, @jonathanolson was going to discuss it with the QA team. What is the status on that?

jonathanolson commented 7 years ago

Discussing with @phet-steele now.