Fix and optimize NumberAbbreviator and its tests

[danielgomezrico]

my Habitica User-ID: 6a5dcf62-46d7-41f6-b4a5-19b1bc8a3784

Tests are not passing on main so this PR will:

• Remove "performance tests" since the approach does not seems right to me, on faster machines they will run faster and so the current approach is flacky, WDYT?
• Remove unused context parameter from the functions

Can you add hacktoberfest-accepted label to this PR please?

[Hafizzle]

Hey @danielgomezrico,

Thanks a lot for your PR! We really like how you've streamlined the NumberAbbreviator. It definitely makes things cleaner and simpler.

Just a heads-up, we usually pick up PRs that are linked to our "Help Wanted" issues. Your PR wasn't attached to one, but we still see the value in what you've done. There are a couple of things though:

Dropping those performance tests means we might not catch how well the function works on different kinds of devices - could we find a way to keep an eye on performance without those tests?
We're also thinking about how to handle different languages without the context parameter.

If you could tweak these bits, it'd be awesome. btw - for your future contributions, it would be awesome if you could check out our 'Help Wanted' list. We've got a bunch of stuff there that could really use your expertise.

Thanks again for jumping in and helping out!

[danielgomezrico]

I was thinking... maybe adding an env variable that defines the time that it should wait, so we can setup it on CI with a realistic value here and have a faster local value, WDYT?

[jsoberg]

FWIW the completes quickly test also fails on my local machine (Ryzen 3700X, ~2500 nanoseconds vs the set 1500 limit). It's difficult to unit test performance with arbitrary timings - is there a specific concern around this that we want to verify the performance with a strict timing?

I'm just starting to look now and so am unfamiliar with the code, but it looks like the number abbreviator is used in single-use contexts with some views instead of being used in bulk. If this is true, in my experience I wouldn't be too worried about this unless it was taking many milliseconds to run, but it looks like the current average duration check is much lower than that (2000 nanoseconds, which is 0.002ms). If we wanted to keep the time check here I'd think that making this duration much higher (.5-1ms) could be acceptable unless I'm missing somewhere in code where this could potentially lock the main thread at that performance level.

[Hafizzle]

Hi @danielgomezrico and @jsoberg,

Thanks for your recent contributions and the discussion around the performance testing concerns. Y'all insights and the suggestions you've both provided are highly valuable.

@danielgomezrico , the idea of using an environment variable for setting different timing thresholds could be beneficial for balancing our CI and local testing needs.

@jsoberg , your analysis on the performance tests, especially considering the specific use of the number abbreviator, has brought up some important considerations about our testing approach. If we were to keep the time check, making the duration higher (The .5-1ms) would actually be fine I think.

There are very real merits in these changes, however we do want to prioritize work with existing 'Help Wanted' issues. Just to be supes-clear though, this is worth revisiting in the future, particularly as we continue to refine our testing strategies and performance benchmarks.

Thanks again for y'all understanding and for your willingness to contribute - it's very much appreciated

[jsoberg]

Thanks @Hafizzle, that makes sense to me!