search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

ComboBox voicing is causing stringTest=long to fail. #739

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

I'm trying to test Geometric Optics with ?stringTest=long. It's failing in ComboBoxListItemNode.js, at this assertion:

65    assert && assert( options.comboBoxVoicingNameResponsePattern.includes( '{{value}}' ), 'value needs to be filled in' );

Testing with other sims that have a ComboBox results in the same failure.

Git history shows this line was added by @zepumph on 2/11/22 in https://github.com/phetsims/sun/commit/8e1b1ad4275d3b3dbef79d7adab62ac97a279ee3 for https://github.com/phetsims/sun/issues/726.

pixelzoom commented 2 years ago

This assertion is going to fail with any stringTest, because stringTest replaces ALL strings, and "{{value}}" will therefore not exist. I'm going to comment it out (and commit it) so I can proceed with my testing.

pixelzoom commented 2 years ago

Question: The purpose of stringTest is to test layout. If voicing strings are not displayed, then should voicing strings be skipped by stringTest? If they were in a standard place in the JSON, it would be relatively easy to skip them.

zepumph commented 2 years ago

Thanks for the report, and sorry that you ran into this.

I didn't do enough research for this assertion, but I felt like the assertion was worth while, and that there was precedent when we have common code that fills in a value.

https://github.com/phetsims/scenery-phet/blob/f1cd444a7c6b0487b31ff9b96b5ef27f3e306234/js/NumberDisplay.js#L113-L121

Perhaps it is as easy as disregarding the assertion when stringTest is provided. Does that sound reasonable to you @pixelzoom? I could also see just removing the assertion, but that doesn't seem like a win to me.

I also noticed that the default value should be SunConstants.VALUE_NAMED_PLACEHOLDER instead of a new i18n string.

I committed above after testing with ?stringTest. How's that?

pixelzoom commented 2 years ago

Changes look good.

You didn't comment on:

If voicing strings are not displayed, then should voicing strings be skipped by stringTest?

... so let's continue that in https://github.com/phetsims/chipper/issues/1198.

Closing.