Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stream lookup with 'undefined' stream ID #1552

Merged

Conversation

sferra
Copy link
Contributor

@sferra sferra commented Feb 13, 2024

The current queue logic attempts to look-up an (undefined) track ID even if no tracks are available.
Such a look-up doesn't make any sense and the queue behavior should be actually specified for such a case.

I'm willing to fix, refactor the queue functionality and add test coverage if someone is able to provide guidance as to what the correct queue behavior would be.

For now, I just added a first test proving the problematic execution path leading to streamProvider.getStreamForId(undefined) .

@nuki-chan
Copy link

nuki-chan bot commented Feb 13, 2024

Lines 1-67 in queue.test.ts

Nuki sees what you did there, adding tests is like adding sprinkles to a cake 🎉, but let's make sure the sprinkles are evenly distributed, ne? 😜

  • Uh-oh! Nuki noticed you're being a sneaky ninja by not initializing your queueItems array properly on lines 37-41. Accessing an array index that has not been initialized—like doing queueItems[trackIndex] = {...}—is like expecting a blind date to be an anime character. Disappointment is imminent! Instead, make sure to initialize queueItems with proper length and perhaps use Array.fill, okay?
const queueItems = Array(trackIndex + 1).fill(undefined);
queueItems[trackIndex] = {
  artist: 'Artist Name',
  name: 'Track Name',
  local: false,
  streams: null
};

Lines 49-49 in queue.ts

Whoa there, coding cowboy! 😘 Nuki sees you exporting getSelectedStreamProvider right from the get-go. Nice move keeping things accessible, but remember to keep it as neat and clean as our beloved codebase-san deserves!

Since Nuki is all about best practices 💅, you should totally consider explaining in a comment why this function needs to be exported. It’s like telling someone why you brought Pocky to the party. The answer's obviously because Pocky is awesome, but the courtesy is appreciated! 😇

// Exporting for use in testing or other action creators that may require 
// the stream provider selection logic.
export const getSelectedStreamProvider = (getState) => {
   // ...existing code...
};

Nuki's work here is done! Keep your code kawaii, and never forget to test like there's no tomorrow~ (because for code without tests, there really might not be! 😱💔)

@nukeop nukeop added the under review This pull request is being reviewed by maintainers. label Feb 14, 2024
Copy link

codecov bot commented Mar 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.84%. Comparing base (d01e199) to head (63fa0cc).
Report is 43 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1552      +/-   ##
==========================================
+ Coverage   71.60%   71.84%   +0.24%     
==========================================
  Files         364      364              
  Lines        6713     6742      +29     
  Branches      479      484       +5     
==========================================
+ Hits         4807     4844      +37     
+ Misses       1512     1501      -11     
- Partials      394      397       +3     

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

@nukeop
Copy link
Owner

nukeop commented Mar 2, 2024

I'm not a fan of unit tests. Have you tried testing it by adding the track to the queue, instead of calling actions directly? It should be possible to set it up the same way.

@sferra
Copy link
Contributor Author

sferra commented Mar 2, 2024

Sorry, I don't understand how you want me to test.
I don't know what you mean when you refer to the queue and actions.

The current test can be simplified a bit by excluding the error, but my first intention was to first prove the issue. The test covers a single, very specific execution path since the function is quite big (which is also the reason why the test is as convoluted as it is). As you can see in 9971065, the fix is quite straight-forward. I don't want to test anything more that the code I changed.

I'm open for suggestions. Maybe point me to an existing test which I can use as an example and adapt for this specific case?

@nukeop
Copy link
Owner

nukeop commented Mar 3, 2024

I mean that in Nuclear, I avoid unit tests, unless it's impossible. The default way to test any functionality is by writing integration tests at the level of views or high level containers, which you can see e.g. by inspecting the PlayQueueContainer tests. Do you think this can be moved there?

@sferra
Copy link
Contributor Author

sferra commented Mar 5, 2024

I had a look at PlayQueueContainer , but that component doesn't interact at all with the findStreamsForTrack function. PlayerBarContainer is the only component which triggers the function, but it doesn't have much to do with the queue.

I attempted to set up a test along with the existing tests of PlayerBarContainer but failed miserably. I got a "Maximum update depth exceeded" error message, probably caused by recursive dispatch calls. I can't figure it out. Also PlayerBarContainer calls useStreamLookup (which in turn triggers findStreamsForTrack) immediately and not by UI interaction, so I'm not sure how to perform verification.

I'm at a loss and don't know how to proceed.

@nukeop
Copy link
Owner

nukeop commented Mar 5, 2024

That container interacts with that function via the useStreamLookup hook, which has an effect that depends on the contents of the queue. I forgot the reason why it's there, on the surface it would make more sense if it was in the queue container, but I vaguely remember there was some reason for it (I wrote it 1.5 years ago; I think it's because the player bar coordinates playback, and the stream search is triggered depending on the playback state). It should be possible to set up a test where both the queue and the player bar are rendered and interact. For example, take a look at the mountComponent function in PlayerBarContainer.test.tsx, and try using something like it except with different store state and both components rendered at the same time:

const component = render(<TestStoreProvider
      store={store}
    >
    <PlayQueueContainer />
      <PlayerBarContainer />
    </TestStoreProvider>);

If you can't figure it out, you can push whatever you manage to do to your branch, and I'll see if I can help you finish it (just mark your PR as accepting commits from me). We've collaborated like that many times in the past in Nuclear and it's pretty great, we both learn a lot this way.

@sferra
Copy link
Contributor Author

sferra commented Mar 5, 2024

We can inspect the queue state without needing PlayQueueContainer, which we don't really need to interact with. I actually had a test set up for PlayerBarContainer locally for using mountConmponent and an IMO appropriate store state. The issue was that the queue changed while the container was created because of the the track removal, causing deep recursion, I think. I'll push my local attempts at setting up the test to a branch later and maybe you can give me some pointers as to what's wrong with them.

@sferra
Copy link
Contributor Author

sferra commented Mar 5, 2024

I pushed my local state. Please let me know if you see what's wrong with the test

@nukeop
Copy link
Owner

nukeop commented Mar 6, 2024

Thanks! I'll try to take a look at this today or tomorrow.

@nukeop
Copy link
Owner

nukeop commented Mar 8, 2024

Ok, I was able to reproduce the infinite loop if I use waitFor. Debugging

@nukeop
Copy link
Owner

nukeop commented Mar 8, 2024

I found out that there's an item placed at index -1 in the queue array, and that's why there's an infinite loop.

@nukeop
Copy link
Owner

nukeop commented Mar 8, 2024

Ok, I fixed it. You were missing an uuid for that test track. After adding it, the test works.

@nukeop nukeop removed the under review This pull request is being reviewed by maintainers. label Mar 8, 2024
@sferra
Copy link
Contributor Author

sferra commented Mar 8, 2024

Thank you!

It wasn't obvious to me that the ID was required for tracks in the queue.
Which part of the code actually triggered the loop? That's also not obvious to me.
Can we improve the code triggering the loop in any way so that next time this happens we at least have some pointer as to why it happens or even break the loop?

As to how we proceed with the PR: should I now remove the unit test or rather leave them in place?
I understand that you prefer testing the behavior of the components, but IMO unit tests have the advantage of making the code more robust and organized (otherwise it can't be tested).

Let me know what you decide, and also if you see anything else which needs to be done.

@nukeop nukeop merged commit 15ae1eb into nukeop:master Mar 9, 2024
9 checks passed
@nukeop
Copy link
Owner

nukeop commented Mar 9, 2024

I went ahead and merged it, no harm in having more thorough test coverage. Thanks for contributing this.

This was happening because the action+reducer combo updates the tracks by uuid. No uuid, no update. Normally I just reuse the same state from storeBuilders.ts, which already includes uuids for the starting queue. Instead of initializing a custom state in tests that require it completely from scratch, a cleaner solution (that I use at work) would be to provide a set of builders that initialize all of the required fields for a given structure, and allow overwriting them.

@sferra
Copy link
Contributor Author

sferra commented Mar 9, 2024

I'm afraid that we were too much in a hurry with merging.
We can change the "to be" in this line to any numeric value and the test doesn't fail as waitFor is async:

 waitFor(() => expect(state.queue.queueItems.length).toBe(0));

I'm afraid that the test doesn't actually test anything. Please correct me if i'm wrong.

@nukeop
Copy link
Owner

nukeop commented Mar 9, 2024

Oops, this is why you do red-green-refactor. I'm not releasing yet though, so you can just keep pushing to this branch, and I can merge the rest at some later point.

If I add await to the test, it fails. I'll see if I can fix it.

@nukeop
Copy link
Owner

nukeop commented Mar 9, 2024

Though when I'm debugging this, all the actions are dispatched correctly and in order, the track should get removed from the queue.

@nukeop
Copy link
Owner

nukeop commented Mar 9, 2024

Ok, this was pretty easy, the store needs to be reloaded after each check.

@sferra
Copy link
Contributor Author

sferra commented Mar 9, 2024

When debugging we usually introduce delays through the debugger and breakpoints and such affect the duration of async operations. It's just a thought.

At least we have the unit test, which tells us that the removal logic is working.

I'm curious as to how the test can be fixed as I have no idea on how to proceed (I'm completely new to TS, React and this whole ecosystem).

@sferra
Copy link
Contributor Author

sferra commented Mar 9, 2024

Ok, this was pretty easy, the store needs to be reloaded after each check.

Makes sense, when you think of it. : )

@nukeop
Copy link
Owner

nukeop commented Mar 9, 2024

I pushed it directly to master, so if you're interested what it looks like, you can see it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants