fix(ephemerals): don't cache empty ephemerals

[Aphosis]

Closes #2173

Ephemerals were initially exposed to package commands as an ephemerals binding.

Since dcf77a2, they are also exposed to late-bound functions using the same strategy: reusing the existing list of resolved ephemerals.

The issue lies in the difference in timing between late-bound functions and package commands:

• Package commands are executed after the context have been resolved, so we get a properly resolved list of ephemerals
• Late-bound functions are executed during a solve, which means we don't have access to the resolved ephemerals yet

The previous implementation did not crash when accessing ephemerals in late-bound functions: instead, it defaulted to an empty list.

This behavior leads to a second issue: ephemerals were added and cached as part of the list of pre-resolve bindings.

This can lead to the following scenario:

• A package late-bound function accesses the ephemerals binding
• Since the context is not resolve yet, no resolved ephemerals is found and an empty list is returned and cached
• Later, in the package commands, we also access ephemerals
• The cached empty list is returned, meaning that most queries will likely fail

This commit addresses these issues the following way:

• Move ephemerals out of pre-resolve bindings for commands, so that they are always retrieved from the resolved context
• For late-bound functions, use resolved ephemerals only if a context is available

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Aphosis / name: Aphosis (6707d24)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.30%. Comparing base (a4160c2) to head (6707d24).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2174   +/-   ##
=======================================
  Coverage   61.29%   61.30%           
=======================================
  Files         164      164           
  Lines       20568    20572    +4     
  Branches     3575     3575           
=======================================
+ Hits        12607    12611    +4     
  Misses       7089     7089           
  Partials      872      872           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Aphosis]

I didn't add any test yet because I'm unsure where it should be added in the test suite, and how the test package should be created.

A good candidate seemed to be in the TestContext class:

rez/src/rez/tests/test_context.py

Line 23 in aa10016

class TestContext(TestBase, TempdirMixin):

Also, the test requires a package with specific attributes, and I'm not sure if the package should be created inline in the test or stored somewhere under data/tests.

[maxnbk]

A good candidate seemed to be in the TestContext class:

I would agree. The bug is about ephemerals being empty inside commands() during a ResolvedContext execution, which makes it a context execution test, rather than a rex binding test or a package attribute test. The existing methods in that class already test patterns like "resolve a context, execute commands, check the environ", which is exactly what needs doing to test this.

Also, the test requires a package with specific attributes, and I'm not sure if the package should be created inline in the test or stored somewhere under data/tests.

IMHO ( @JeanChristopheMorinPerso might disagree), an inline-package created into a tempdir, resolving against it, and checking the environ is preferable to a fixture under data/tests because the pkgdef is right in the test, we can easily see what late() / commands() are set to, and what is being checked without digging into data/tests. The existing late_binding fixture is for testing package-attribute-access, not context-execution, so adding it there would probably blur the line between the package resource tests (test_packages.py) and context execution tests (test_context.py). The test also requires the specific ephemeral request, and so separating the pkgdef from the test would make the relationship between the two less clear.

A few specifics to help guide you to a working test that tests what you want it to test:

• Use self.root for the temp directory (TempdirMixin is being used so it's a managed temp dir that gets clean up automatically)
• Use get_environ() and not execute_command() to avoid annoying subprocess plumbing or parent_environ e.g. the way that text_execute_command_testing_environ does it.
• Assert both the truthiness of r.resolved_ephemerals and that the environ flag is "1" to catch both the case where the text fixture is wrong and no ephemeral resolves, and the actual bug itself
• Comment that the late function is there to trigger the bug, not to test the requires behavior.

I would also advise a second test that goes without the late-function as a control, which should also see the ephemeral, but is what proves the fix doesn't break the normal path while demonstrating that the late function was the trigger for the bug.

[maxnbk]

I am just marking this "Request Changes" so that review can be re-requested when updates are made, so that I'll see it again.

[JeanChristopheMorinPerso]

IMHO ( @JeanChristopheMorinPerso might disagree), an inline-package created into a tempdir, resolving against it, and checking the environ is preferable to a fixture under data/tests because the pkgdef is right in the test

FYI @maxnbk I'm usually in the camp of "no test data file and everything must be inline with the test to easily see what's being tested". Though, I know that this can sometimes incur some costs, but I don't think there's any costs involved here.