Showing that the current version fails the null_pack_color_test.ts

[noah-rarick]

This draft pull request shows the initial failing test null_pack_color.test.ts. packColor is a function called in program_configuration.ts that breaks rendering when passed a null color. This test should prove that packColor cannot handle null values.

Not sure if exporting this function is bad practice for testing.

#4234

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.04%. Comparing base (b8cb683) to head (7043a5f).

❗ There is a different number of reports uploaded between BASE (b8cb683) and HEAD (7043a5f). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (b8cb683)	HEAD (7043a5f)
	9	8

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #4351       +/-   ##
===========================================
- Coverage   87.85%   64.04%   -23.82%     
===========================================
  Files         246      246               
  Lines       33372    33372               
  Branches     2182     1514      -668     
===========================================
- Hits        29318    21372     -7946     
- Misses       3064    11162     +8098     
+ Partials      990      838      -152     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Thanks for taking the time to open this PR!
While this is the symptom I doubt it's the root cause of this issue.
I would recommend trying to build a test that simulates the bug, or at least something a bit "bigger" in terms of scope that doesn't require exporting methods that were not exported previously.
If, at the end, you come to the conclusion that this is really the problem and you would like to unit test this method, then this might be the right approach, but I would expect a detailed investigation results prior to it.
Another approach might be to call this method only when the color is not null or making sure the color is always defined. Those are completely different solutions obviously...

[noah-rarick]

So just so I understand: even though I know that this prevents the rendering from breaking, I should look at other approaches that solve this issue in a more elegant way?

I assume this means I should sift through the call stack and look for where the logic went wrong at a high level, instead of focusing on this very granular issue.

[HarelM]

Yes please. Returning a color when no color is requested can be cumbersome in certain cases, I think.

[noah-rarick]

#4234 I still believe that the packColor function is causing this rendering issue. I added a render test from #4241 where user qpincon displayed this issue. Locally, I can pass the rendering tests by employing my fix which adds a null value check within the packColor function.