unitsofmeasurement / uom-systems

Units of Measurement Systems
http://www.uom.systems
Other
36 stars 17 forks source link

Create unit tests for UCUM formatter #81

Closed magnonasc closed 7 years ago

magnonasc commented 7 years ago

After fixing the problem with the formatter in #21 for the specific m/s case we need to create tests to make sure that it works for every other different format possible

magnonasc commented 7 years ago

In advance I can tell that I'll change most of the units used in systems.uom.ucum.format.UnitFormatTest, because it was using most METRE (not METER), KILOMETRE_PER_HOUR and some other units that are not on scope of UCUM, so this will make sure that the formatter is working correctly for the correct units.

keilw commented 7 years ago

Well UCUM does not define speed units per se, but while "seconds" is used in a combined unit, another "non-unit" is gram meter per heartbeat. So in most cases UCUM also sticks to singular like ICU4J/Unicode, which is why we adjusted it in other systems like Unitsor SI. SI, e.g. http://www.bipm.org/en/publications/si-brochure/section2-2.html is also quite singular, newton per metre,radian per second,...

magnonasc commented 7 years ago

But talking within the scope of UCUM, we should only test their units... For example, if you have:

    @Test
    public void testFormat4() {
        Unit<Speed> kph = Unit.KILOMETRE_PER_HOUR;
        assertEquals("km/h", kph.toString());
    }

in systems.uom.ucum.format.UnitFormatTest and

    @Test
    public void testFormat2() {
        Unit<Speed> kph = Unit.KILOMETRE_PER_HOUR;
        assertEquals("km/h", kph.toString());
    }

in si.uom.UnitFormatTest, where both KILOMETRE_PER_HOUR are the same unit, what are you really testing from UCUM? That's what I'm saying, as we have tests for that in other modules, we need to test in UCUM the units of UCUM only, like KILO(METER).divide(HOUR) or use some other UCUM's composed unit, which is what we can have in UCUM. If not, there's no reason to have such a test.

Can you see what I mean?

magnonasc commented 7 years ago

I'm confused about why PASCAL.toString() shows PA while PLANCK.toString() shows J.s*6.6260755E-34, something must be changed?

keilw commented 7 years ago

Because PASCAL is derived fromUnits.PASCAL, while PLANCK did not exist on the implementation level. So you should be able to get the right string via UCUMFormat, but AbstractUnit.toString()does not know about that as mentioned earlier with the JSR 310 analogy.

keilw commented 7 years ago

Please try again now after pulling the UCUM system. I noticed the UCUM demos are not in the repo yet, will dig them up and add them, but what you tested with toString() should work for PLANCK now, too.

There is a TODO (feel free to generate further issues from it if you want) noting it probably should be done a little more generic, but the static initializer of UCUM then has to go through all the resource bundle entries (in UCUMFormat.PRINTI'd say)

keilw commented 7 years ago

I believe there's more to do, as https://github.com/unitsofmeasurement/uom-systems/issues/96 showed. We could also create smaller tasks per table/group of the UCUM spec, but for now I reopened it.

garretwilson commented 7 years ago

Yeah, I'm still trying to understand how this made it into the final release. I had mentioned at https://github.com/unitsofmeasurement/uom-systems/issues/82#issuecomment-289134536:

Lastly I want to say that it is imperative that the following work:

UnitFormat ucumUnitFormat;  //TODO get unit format for "UCUM"
assertThat(ucumUnitFormat.format(ucumUnitFormat.parse("L")), is("L"));

I believe @magnonasc added that test. And you know what? That test passes with 0.7!!! But this one would fail with 0.7:

assertThat(ucumUnitFormat.format(ucumUnitFormat.parse("uL")), is("uL"))

So 0.7 thinks "L" is "L", but thinks "uL" is "nst". That's just crazy. That sort of thing shouldn't be happening. How do I know in the future that the library won't arbitrarily change some unit just because I use m or u or c or something similar as a prefix?

keilw commented 7 years ago

Closing this for now, there are no resources for additional tests.