[PER-10319] Fix flaky test in data service

[aasandei-vsp]

The method fetchFullItems in the data service chains multiple promises, which normally wouldn't be a problem. But in this case, we do not get the modified records at the end of chaining the promises, we actually just modify the reference to the records list we sent as an argument. This implementation yelds unpredictable results, so that is why adding a timeout is the solution here, to make sure that the records have been assigned the new value before checking.

Issue PER-10319 Fix flaky test in data service

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.03%. Comparing base (92e0821) to head (7b34d49).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
+ Coverage   46.98%   47.03%   +0.05%     
==========================================
  Files         372      372              
  Lines       11494    11494              
  Branches     1907     1907              
==========================================
+ Hits         5400     5406       +6     
  Misses       5905     5905              
+ Partials      189      183       -6     

☔ View full report in Codecov by Sentry.
📢 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cecilia-donnelly]

This seems reasonable. Thanks for the clear explanation in the commit message!

I wonder if there's a more complete fix that wouldn't rely on a timeout - are we just modifying the reference specifically because these are placeholders? That is, does this seem like the right solution to you?

[aasandei-vsp]

It does not seem the right solution to me, for sure.
The problem here is the method we are testing.
In my opinion this is what needs to change, in order to be able to use and test it reliably.
What I can do is create a step by step description of what the method is doing, to try and maybe identify a better way of testing it.
Ideally, we should refactor the method, so the return is what we rely on, not a reference we happen to pass as an argument.
I'd still merge this as a partial "fix", so the test does not fail anymore.
@slifty @cecilia-donnelly I totally agree that my fix is more of a hack, but it would be a fast way to just get rid of the failing test. What I would do is add a ticket for refactoring/finding a better solution to test the method at hand. What's your opinion on this?

[Copilot]

Pull Request Overview

This PR attempts to fix a flaky test in the data service by converting it from callback-based to async/await pattern and adding a 3-second timeout before checking assertions. The change aims to address timing issues where modified records may not be immediately available after the fetchFullItems promise resolves.

• Converted test from callback-based (done) to async/await pattern
• Added 3-second setTimeout before checking record assertions

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The setTimeout callback is not awaited, so the test will complete before the assertions inside setTimeout execute. This means the assertions will never run, and the test will always pass regardless of the actual behavior. To properly wait for asynchronous operations, either wrap setTimeout in a Promise and await it, use await new Promise(resolve => setTimeout(resolve, 3000)) followed by the assertions, or better yet, use fakeAsync and tick from Angular testing utilities to avoid arbitrary timeouts.

[aasandei-vsp]

That is actually not true. The test does not always pass, the "magic number" I chose is to make sure that the property I have no access to will surely be populated. If I chose a lower value, the test would again pass/fail, because there was not enough time to make sure the unpredictable property is populated.

[Copilot]

A 3-second timeout significantly slows down test execution. If a delay is truly necessary, consider using Angular's fakeAsync and tick utilities to avoid real time delays, or investigate the root cause of the timing issue in fetchFullItems to fix it properly rather than masking it with an arbitrary timeout.

[aasandei-vsp]

I have tried, but because there is no real control over the async population of the records reference, none of these solutions would work.
It is true, the real issue is that we do not return a new value, we just modify a reference we're getting as a parameter, big no no in any programming language.

[Copilot]

The 3000ms delay is a magic number without explanation. If this approach is kept, consider extracting it to a named constant with a comment explaining why this specific delay is needed, though addressing the root cause of the timing issue would be a better solution.

[aasandei-vsp]

Indeed, the solution is hacky and 3000ms is just by trial and error to make sure that the records reference has been updated, because we do not have real control over the way that is being changed in an async way.

[slifty]

It does seem potentially worth adding a comment to that effect, just so we don't lose the trial and error.

[slifty]

I join you in being VERY excited at the idea of fixing the flaky test -- that said I do think we should fix the underlying issue.

If that's too big a lift for now I think it might be cleaner for us to disable this test until the underlying race condition is fixed, (as opposed to incorporating timeouts in the test).

[aasandei-vsp]

@slifty @cecilia-donnelly The 2 approaches:

1. Leave this hack with the timeout, because at least we are testing some part of the function
2. Disable the test, because this is indeed a bigger problem. The method that we are testing needs a serious refactoring.

My issue here is that refactoring this method is high risk, low gain. It is used in several places and to be honest, I am not even sure why it works at all times, taking into account that the results are unpredictable. Probably the luck of being used in async flows and in the view.

[slifty]

@aasandei-vsp good summary! I am personally comfortable with either approach. To your point, having some test is ultimately better than no test.

To the point you made earlier in this PR thread: I do think we it would be good to make an issue in this repo to capture what you have learned so far / the need for refactoring the method), and then reference that issue in a comment around this test. (whether you disable the test or leave the modified version).

[cecilia-donnelly]

Agreed with what @slifty wrote and your direction, @aasandei-vsp : some test is better than no test. I like the idea of making an issue to capture everything you've learned!