Running test fails #256

Closed
opened 2025-12-30 01:22:50 +01:00 by adam · 9 comments
Owner

Originally created by @StefMa on GitHub (Dec 20, 2024).

Hey guys,
greetings from Germany. We typically write numbers with , instead of . 🤓
1,20 Euro
1.20 Euro.

Why is this interesting? Cause right now I get errors when running

./gradlew test
expected: 
  "module test
    facts
      ✘ fail
         4 == 9 (/tempDir/test.pkl, line xx)
  
  0.0% tests pass [1/1 failed], 50.0% asserts pass [1/2 failed]
  "
 but was: 
  "module test
    facts
      ✘ fail
         4 == 9 (/tempDir/test.pkl, line xx)
  
  0,0% tests pass [1/1 failed], 50,0% asserts pass [1/2 failed]
  "

Right now these tests are failing:

CliTestRunner fail test(Path)
CliTestRunner succeed test(Path)
CliTestRunner with thrown error in examples -- existing expected output(Path)
CliTestRunner with thrown error in examples -- no expected output(Path)
CliTestRunner with thrown error in facts(Path)
example length mismatch(Path)
Originally created by @StefMa on GitHub (Dec 20, 2024). Hey guys, greetings from Germany. We typically write numbers with `,` instead of `.` 🤓 ✅ 1,20 Euro ❌ 1.20 Euro. Why is this interesting? Cause right now I get errors when running ``` ./gradlew test ``` ``` expected: "module test facts ✘ fail 4 == 9 (/tempDir/test.pkl, line xx) 0.0% tests pass [1/1 failed], 50.0% asserts pass [1/2 failed] " but was: "module test facts ✘ fail 4 == 9 (/tempDir/test.pkl, line xx) 0,0% tests pass [1/1 failed], 50,0% asserts pass [1/2 failed] " ``` Right now these tests are failing: ``` CliTestRunner fail test(Path) CliTestRunner succeed test(Path) CliTestRunner with thrown error in examples -- existing expected output(Path) CliTestRunner with thrown error in examples -- no expected output(Path) CliTestRunner with thrown error in facts(Path) example length mismatch(Path) ```
adam closed this issue 2025-12-30 01:22:51 +01:00
Author
Owner

@odenix commented on GitHub (Dec 20, 2024):

Other things that could be improved about this message: don’t mix present and past tense, don’t use percentage for passed tests and fraction for failed tests. Not sure the assert stats are useful.

@odenix commented on GitHub (Dec 20, 2024): Other things that could be improved about this message: don’t mix present and past tense, don’t use percentage for passed tests and fraction for failed tests. Not sure the assert stats are useful.
Author
Owner

@bioball commented on GitHub (Dec 20, 2024):

Hm, that's interesting.

It's probably a good thing that our test report formats numbers that are locale-specific, but our Grade build should nevertheless be consistent.

For now, you should be able to get the tests passing with LC_ALL=en_US.UTF-8 ./gradlew test.

Other things that could be improved about this message: don’t mix present and past tense, don’t use percentage for passed tests and fraction for failed tests. Not sure the assert stats are useful.

Yeah, I agree, the summary can be polished up a bit. But, the number of assertions is meaningful for some users (CC @jjmaestro).

@bioball commented on GitHub (Dec 20, 2024): Hm, that's interesting. It's probably a good thing that our test report formats numbers that are locale-specific, but our Grade build should nevertheless be consistent. For now, you should be able to get the tests passing with `LC_ALL=en_US.UTF-8 ./gradlew test`. > Other things that could be improved about this message: don’t mix present and past tense, don’t use percentage for passed tests and fraction for failed tests. Not sure the assert stats are useful. Yeah, I agree, the summary can be polished up a bit. But, the number of assertions is meaningful for some users (CC @jjmaestro).
Author
Owner

@jjmaestro commented on GitHub (Dec 20, 2024):

@odenix my rationale when adding this was that the % was a nicer metric for pass because you always want that to be 100% and knowing the exact number of tests that fail was a good metric for fail since that's the stuff you need to fix. Likewise for asserts since that also tells you the work that needs doing (fail) and the amount of testing you actually have (e.g. it's not the same having 1 test with 1 assert VS 1 test with a lot of asserts, the second case usually covers more corners so it's nice to know as a metric).

Plus, it doesn't hurt just having some summary at the end of the test run, right? :) Most test frameworks report these types of metric!

@jjmaestro commented on GitHub (Dec 20, 2024): @odenix my rationale when adding this was that the % was a nicer metric for `pass` because you always want that to be `100%` and knowing the exact number of tests that fail was a good metric for `fail` since that's the stuff you need to fix. Likewise for `asserts` since that also tells you the work that needs doing (`fail`) and the amount of testing you actually have (e.g. it's not the same having 1 test with 1 assert VS 1 test with a lot of asserts, the second case usually covers more corners so it's nice to know as a metric). Plus, it doesn't hurt just having some summary at the end of the test run, right? :) Most test frameworks report these types of metric!
Author
Owner

@odenix commented on GitHub (Dec 20, 2024):

It's probably a good thing that our test report formats numbers that are locale-specific

I don't think that English messages should use German number formatting. It would, of course, make sense in German messages.

@odenix commented on GitHub (Dec 20, 2024): > It's probably a good thing that our test report formats numbers that are locale-specific I don't think that English messages should use German number formatting. It would, of course, make sense in German messages.
Author
Owner

@StefMa commented on GitHub (Dec 20, 2024):

Honestly I don't know why this happen. My MacBook is set up in English/US, Keyboard is set up to English/US. My IDE is in English/US. I am "only" located in Germany. So I'm also super curious why this even happened.

I am with @odenix here. This is a developer tool, I guess it should stick to english defaults and shouldn't introduce local-specifics.

@StefMa commented on GitHub (Dec 20, 2024): Honestly I don't know why this happen. My MacBook is set up in English/US, Keyboard is set up to English/US. My IDE is in English/US. I am "only" located in Germany. So I'm also super curious why this even happened. I am with @odenix here. This is a developer tool, I guess it should stick to english defaults and shouldn't introduce local-specifics.
Author
Owner

@bioball commented on GitHub (Dec 23, 2024):

Is the decimal different based on the language being used (as opposed to locale)? I don't have strong opinions here; we can use period decimals consistently if that's the norm.

@bioball commented on GitHub (Dec 23, 2024): Is the decimal different based on the language being used (as opposed to locale)? I don't have strong opinions here; we can use period decimals consistently if that's the norm.
Author
Owner

@odenix commented on GitHub (Dec 23, 2024):

A locale is a combination of language and country/region. By honoring the user's locale but displaying messages in English, we effectively change their locale to an unusual combination of language and country/region, which can be confusing.

It sounds like @StefMa has his locale set to en-US, but apparently his Pkl executable thinks it's en-DE or de-DE.

@odenix commented on GitHub (Dec 23, 2024): A locale is a combination of language and country/region. By honoring the user's locale but displaying messages in English, we effectively change their locale to an unusual combination of language and country/region, which can be confusing. It sounds like @StefMa has his locale set to en-US, but apparently his Pkl executable thinks it's en-DE or de-DE.
Author
Owner

@bioball commented on GitHub (Dec 23, 2024):

Ah! I didn't realize "locale" was a technical term. My question was, really, does the decimal change based on language and not region?

To answer that: doesn't look like it does. According to LocalePlanet, the decimal for en-DE is ,.

But, yeah, I agree that we're mis-formatting numbers here. All of our messages are written in en-US (Pkl currently does not change its messages based on locale), so changing the decimal here is a mistake.

@bioball commented on GitHub (Dec 23, 2024): Ah! I didn't realize "locale" was a technical term. My question was, really, does the decimal change based on language and not region? To answer that: doesn't look like it does. According to [LocalePlanet](https://www.localeplanet.com/icu/en-DE/index.html), the decimal for en-DE is `,`. But, yeah, I agree that we're mis-formatting numbers here. All of our messages are written in en-US (Pkl currently does not change its messages based on locale), so changing the decimal here is a mistake.
Author
Owner

@StefMa commented on GitHub (Dec 24, 2024):

I opened a PR to fix that issue.

The discussion what do display here and in which form can be discussed somewhere else.
I guess it is important to make this stable first.

@StefMa commented on GitHub (Dec 24, 2024): I opened a [PR](https://github.com/apple/pkl/pull/868) to fix that issue. The discussion what do display here and in which form can be discussed somewhere else. I guess it is important to make this stable first.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/pkl#256