Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common code unit tests are not run in PhET-iO brand #191

Closed
zepumph opened this issue Aug 12, 2020 · 8 comments
Closed

Common code unit tests are not run in PhET-iO brand #191

zepumph opened this issue Aug 12, 2020 · 8 comments

Comments

@zepumph
Copy link
Member

zepumph commented Aug 12, 2020

Looking through listContinuousTests.js, I don't see a way that common code is being tested in phet-io brand. This is because repos like tandem and axon are not in perennial/data/phet-io, as they aren't runnable phet-io repos. For right now, I will just manually add them to make sure they get unit tested in phet-io brand.

@zepumph
Copy link
Member Author

zepumph commented Aug 12, 2020

After the above commit, I'm pretty sure that axon is going to start failing on CT, as that was what I found to be the case when beginning to work on phetsims/axon#316, but it is better for it to be load failures instead of quiet ones.

Tagging @jonathanolson to make sure that it is alright to use assert in listContinuousTests.js as I did above.

Tagging @samreid and @chrisklus to note that we have failing axon unit tests in phet-io brand (another issue coming soon). And also to note that this is likely not complete, we my want to brainstorm more complete solution.

@zepumph
Copy link
Member Author

zepumph commented Aug 12, 2020

Also, I wonder if this would be solved by adding phet-io unit tests to pre-commit hooks.

@zepumph
Copy link
Member Author

zepumph commented Aug 12, 2020

Here are the tests axon was testing up until this point:

image

@samreid
Copy link
Member

samreid commented Aug 15, 2020

Would it be reasonable to say "all tests run in ?brand=phet-io" and we endeavor to make the phet brand unit tests a subset of phet-io brand unit tests?

@samreid samreid assigned zepumph and unassigned samreid Aug 15, 2020
@zepumph
Copy link
Member Author

zepumph commented Aug 19, 2020

Yes, as long as we don't just run them in PhET-iO brand. Perhaps they could all be run in both on CT? I don't think that would require much work.

I ran the following, and found potentially simple instrumentation errors to fix in scenery, scenery-phet, and sun. Everything else passed:

http://localhost:8080/area-model-common/area-model-common-tests.html?ea&brand=phet-io
http://localhost:8080/area-model-common/area-model-common-tests.html?ea&brand=phet-io
http://localhost:8080/axon/axon-tests.html?ea&brand=phet-io
http://localhost:8080/balloons-and-static-electricity/balloons-and-static-electricity-tests.html?ea&brand=phet-io
http://localhost:8080/circuit-construction-kit-common/circuit-construction-kit-common-tests.html?ea&brand=phet-io
http://localhost:8080/dot/dot-tests.html?ea&brand=phet-io
http://localhost:8080/faradays-law/faradays-law-tests.html?ea&brand=phet-io
http://localhost:8080/fractions-common/fractions-common-tests.html?ea&brand=phet-io
http://localhost:8080/griddle/griddle-tests.html?ea&brand=phet-io
http://localhost:8080/interaction-dashboard/interaction-dashboard-tests.html?ea&brand=phet-io
http://localhost:8080/joist/joist-tests.html?ea&brand=phet-io
http://localhost:8080/kite/kite-tests.html?ea&brand=phet-io
http://localhost:8080/phet-core/phet-core-tests.html?ea&brand=phet-io
http://localhost:8080/phet-io/phet-io-tests.html?ea&brand=phet-io
http://localhost:8080/phetcommon/phetcommon-tests.html?ea&brand=phet-io
http://localhost:8080/scenery/scenery-tests.html?ea&brand=phet-io
http://localhost:8080/scenery-phet/scenery-phet-tests.html?ea&brand=phet-io
http://localhost:8080/studio/studio-tests.html?ea&brand=phet-io
http://localhost:8080/sun/sun-tests.html?ea&brand=phet-io
http://localhost:8080/tandem/tandem-tests.html?ea&brand=phet-io
http://localhost:8080/twixt/twixt-tests.html?ea&brand=phet-io
http://localhost:8080/utterance-queue/utterance-queue-tests.html?ea&brand=phet-io

@zepumph
Copy link
Member Author

zepumph commented Aug 19, 2020

I was a little on the fence thinking about all of the above sims that don't support PhET-iO brand, but especially after the above commit, I think that it is still the way to go.

The above work now makes scenery run successfully in scenery. I would say it was 90% busy work, but it still felt like important clean up. The other 10% were actual PhET-iO tests that were broken and hadn't been run anytime recently. Wahoo!!

Next steps for this issue:

@zepumph
Copy link
Member Author

zepumph commented Aug 20, 2020

@samreid please review in correlation with #174. How do we feel about the assumption that all unit tests support being run in brand=phet-io?

@zepumph zepumph assigned samreid and unassigned zepumph Aug 20, 2020
@samreid
Copy link
Member

samreid commented Aug 21, 2020

Running all unit tests in both brand=phet and brand=phet-io sounds appropriate and in line with the long term vision of our project. For repos that have no special phet-io features, they will run redundant tests, but I believe that is OK because most unit tests are fast to run.

If and only if we decide that is taking too long, then I'd recommend pursuing a strategy where we only (or predominantly) run tests in phet-io brand, and make sure the phet brand tests are run as a subset. For the rare cases where we need to test something in phet-brand, that could be listed as a special case.

I skimmed the commits and didn't have further recommendations. Thanks @zepumph!

@samreid samreid assigned zepumph and unassigned samreid Aug 21, 2020
@zepumph zepumph closed this as completed Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants