Conversation
Security Review ✅
|
9acc2fc to
a02bcc3
Compare
a02bcc3 to
d84f237
Compare
The task lowered the VM-wide Logger level so Ecto's debug query logging wouldn't pollute the hash it prints on stdout, and never put it back. That's harmless from the command line, where the process exits straight after. Called in-process from the test suite it isn't: the level is one value for the whole VM, so later capture_log assertions came back empty and unrelated tests failed depending on run order. start_repo now only quietens the logger when it boots the repo itself. In-process the repo is already running, so there is nothing to boot and no reason to touch the level. The already_started clause goes with it, since the guard answers that question first.
d84f237 to
ba6d6e9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5060 +/- ##
=======================================
+ Coverage 90.6% 90.7% +0.1%
=======================================
Files 422 422
Lines 20043 20043
=======================================
+ Hits 18160 18172 +12
+ Misses 1883 1871 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@stuartc I had a look at #5099. Although they share the same root cause, they need different fixes. #5099 could use Heads up for OFN-4525: the other two files need a third approach again. They turn the level up to catch Can you review? Not urgent :) |
stuartc
left a comment
There was a problem hiding this comment.
Ok after taking a closer look the changes you made do make sense. But the actual original implementation doesn't, and the behavior we're trying to preserve.
So it's a mix task that needs to start the repo, I get that. But testing a mix task directly is odd in hindsight.
TBH, if anything we should be testing the lib side (I assume we are) and if we really want to, expose maybe the function that assumes it's all running, the one that prints stuff out and/or throws an error. But I mean tbh, I wouldn't personally invest anything in testing this. I'm not aware of (not have I tried) a proper approach for testing mix tasks from the outside (without actually calling mix via a shell command).
I'd say dropping the test for the mix command entirely would be an appropriate solution here.
Description
This task turns the logging volume down so its output is just the hash. There is one volume setting for the whole program, and the task never turned it back up.
Run from a terminal, that's harmless. The task prints a hash and the program exits a second later, taking the setting with it.
The test suite is different. It's one long-running program, and it calls this task as an ordinary function. So the task muted logging, and then thousands of later tests ran with it still muted. Any test checking "did this log a warning?" got nothing back, and which tests broke depended on the seed.
The fix is one question. Did this task start the program, or was it already running?
No Repo running means we're a fresh CLI invocation, so the task starts what it needs and quietens the logger as before. A Repo already running means we're a guest inside the test suite, so the task does nothing and leaves the setting alone.
That also makes the old
{:error, {:already_started, _pid}}clause unreachable, so it's gone.The
Logger.configureline has to stay for the CLI. Ecto logs every query at:debug(config/config.exs:19), dev sets no logger level, and Logger writes to stdout, so without it[debug] QUERY OKlines land in the same stream as the hash. In tests it was never doing anything useful, since the suite already runs at:warning.Same root cause as #5099, where the shared setting is the problem, but a different fix. That PR swapped in
with_log, which gets the same quietening scoped to one capture. Here there was nothing worth scoping, since the quietening was useless in tests anyway, so the call is just skipped.Validation steps
mix test test/mix/tasks/gen_workflow_hash_test.exspasses. Against the old version the new test fails withleft: :error, right: :info, so it catches the regression rather than passing by accident.mix testpasses.mix lightning.gen_workflow_hash <uuid>still prints only the hash.Additional notes for the reviewer
resolver_test.exsandworkflow_channel_test.exs. Worth a heads-up for whoever takes that ticket, because the Stop vault_test.exs mutating the VM-wide Logger level #5099 swap won't work on them. Both turn the volume up to catch:info/:debuglogs that the suite's:warningsetting would otherwise drop, andwith_log(level: ...)can't do that. The message is discarded before the capture ever sees it.AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code):owner,:admin,:editor,:viewer)