skyline: Fixed the small-molecule conversion tests racing the library loader

[chambm]

The failure

RefineConvertToSmallMoleculesTest and RefineConvertToSmallMoleculeMassesAndNamesTest fail whichever one runs first in a TestRunner process, and pass on every later run in that process:

TransitionGroupChromInfo results differ:
  YLGAYLLATLGGNASPSAQDVLK Charge 2  vs  pep_YLGAYLLATLGGNASPSAQDVLK Charge [M+2H]
  [0] [0] LibraryDotProduct 0.72051907 vs (null)

Reversing the order moves the failure to the other test, so it is ordering, not mode:

run order	converted .blib written	failed
formulas, then masses_and_names	only masses_and_names	formulas
masses_and_names, then formulas	only formulas	masses_and_names

Cause

RefinementSettings.ConvertToSmallMolecules converts libraries only when the document reports them loaded:

var canConvertLibraries =
    invertChargesMode == ConvertToSmallMoleculesChargesMode.none &&
    mode != ConvertToSmallMoleculesMode.masses_only &&
    document.Settings.PeptideSettings.Libraries.IsLoaded;

That is the right call for a document that never loaded a library. The problem is on the test side: AsSmallMoleculeTestUtil.ConvertToSmallMolecules takes its document from ResultsTestDocumentContainer.AssertComplete(), which only inspects the chromatogram loader's progress and returns while the library manager is still working.

So on a cold cache the conversion reads IsLoaded as false, skips the library branch, and yields a converted document with no library dot product while the original has one. On a warm cache the library is already present and the two agree.

This was a coverage hole, not only a flake

In the runs that failed, no worm.1.1.converted_to_small_molecules.blib was written at all and the converted precursors carried no <bibliospec_spectrum_info>. Library conversion was not being exercised in those runs — the test failed only because the two sides then disagreed about a dot product.

Fix

Wait for the libraries before converting. A document with no libraries reports itself loaded, so this is a no-op everywhere else.

Verification

Reproduced the order dependence, then re-ran both orderings after the change: all orderings pass, and both formulas and masses_and_names now write a converted .blib regardless of order. masses_only still writes none, which is correct — that mode is excluded by the mode != masses_only term.

Built and run on the .NET 10 port branch (#4619), where all three touched files are byte-identical to master; this PR exists to get it exercised on master itself. Every API it uses is already used in the same file on master.

🤖 Generated with Claude Code

https://claude.ai/code/session_0182oRXbeKQGEpF1kTkdQAsr

[Copilot]

Pull request overview

This PR addresses an order-dependent flake in Skyline’s small-molecule conversion tests by ensuring spectral libraries have finished loading before conversion/comparison logic runs, so library conversion paths are exercised consistently.

Changes:

• Added a ResultsTestDocumentContainer.WaitForLibrariesLoaded() helper with a longer timeout tailored to library loading.
• Updated AsSmallMoleculeTestUtil.ConvertToSmallMolecules() to wait for libraries to load after document load completion, closing the race with the library loader.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pwiz_tools/Skyline/TestUtil/ResultsUtil.cs	Adds a dedicated wait helper for spectral library load completion in test document containers.
pwiz_tools/Skyline/TestUtil/AsSmallMoleculeTestUtil.cs	Uses the new wait to ensure libraries are loaded before running small-molecule conversion comparisons.

Suppressed comments (2)

pwiz_tools/Skyline/TestUtil/ResultsUtil.cs:261

• GetWaitCycles() can return 0 when millis < SLEEP_INTERVAL, which causes callers to skip the condition check loop and immediately Assert.Fail() even if the condition is already satisfied. Since this helper is used by the new library-load wait, it should be robust for all inputs.

This issue also appears on line 293 of the same file.

        public const int WAIT_TIME_LIBRARIES = 60 * 1000;    // 60 seconds

        private static int GetWaitCycles(int millis = WAIT_TIME)
        {
            return millis / SLEEP_INTERVAL;

pwiz_tools/Skyline/TestUtil/ResultsUtil.cs:297

• WaitForLibrariesLoaded never re-checks Libraries.IsLoaded after the final Sleep. If the libraries finish loading during the last sleep interval, the method can still fail even though the condition is true by the time it reaches the Assert.Fail(). Add one last check after the loop before failing.

            {
                if (Document.Settings.PeptideSettings.Libraries.IsLoaded)
                    return;
                Thread.Sleep(SLEEP_INTERVAL);
            }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chambm]

Addressed both suppressed suggestions from the Copilot review in 214fc5f.

WaitForLibrariesLoaded never re-checks after the final sleep — fixed. The loop sleeps after its last check, so libraries finishing during that final interval produced a spurious failure. That is the same shape of race this method exists to close, so it was worth getting right.

GetWaitCycles() can return 0 when millis < SLEEP_INTERVAL — covered by the same change rather than separately. The new post-loop check evaluates the condition once regardless of cycle count, so a sub-interval timeout can no longer fail without ever looking.

I deliberately did not harden GetWaitCycles itself. It is shared with the pre-existing WaitForProcessing, so a Math.Max(1, ...) there would change behaviour beyond this PR for a case no caller passes today. Happy to do it if a reviewer prefers the helper fixed at the source.

Re-verified both orderings after the change — formulas-first and masses_and_names-first both pass, and both write a converted .blib.

[chambm]

Not needed with #4629 fixes.

[chambm]

It seems this race still exists even with 4629. This fixes it according to my testing.