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 BaseItemKind conversion for PlaylistsFolder #5228

Open
wants to merge 14 commits into
base: master
from

Conversation

@lbenini
Copy link

@lbenini lbenini commented Feb 13, 2021

Return the correct ClientTypeName to allow Enum Parse
Add dynamic unit tests to ensure all BaseItem concrete descendants have an associated BaseItemKind

Changes
Following the merge of #5209 and the introduction of the BaseItemKind enum has introduced the assumption that each concreate BaseItem descendant has a type name present in the enum or that overrides GetClientTypeName and returns a string that can be parsed as the enum.
To verify automatically that this assumption is honoured the BaseItemKindTests are implemented and the test is dynamically applied to all concrete classes that inherit from BaseItem

Issues
#5209 on PlaylistsFolder
Fixes #5269

matthin and others added 5 commits Feb 10, 2021
Return the correct ClientTypeName to allow Enum Parse
Added dynamic unit tests to ensure all BaseItem concrete descend
@crobibero
Copy link
Member

@crobibero crobibero commented Feb 13, 2021

Thanks for making a test for all of the possible BaseItemKinds!
I think the tests should be added to Jellyfin.Controller.Tests instead of creating a new testing project

@lbenini
Copy link
Author

@lbenini lbenini commented Feb 13, 2021

I think the tests should be added to Jellyfin.Controller.Tests instead of creating a new testing project

The reason to create a test project depending on the executing assembly is to be sure to catch all instances of the classes:
The BaseItem is inherited in multiple assemblies, so the same test should be reimplanted in Jellyfin.Controller.Tests and in a new project to cover the uses in Emby.Server.Implementation.

So instead of adding a new project (Emby.Server.Implementation.Tests) and duplicate the test, I've added a single project and single test.

@Bond-009
Copy link
Member

@Bond-009 Bond-009 commented Feb 13, 2021

Jellyfin.Server.Implementation.Tests is the correct place, Jellyfin.Server is only for application startup handling it'll never contain a baseitem

@lbenini
Copy link
Author

@lbenini lbenini commented Feb 13, 2021

Jellyfin.Server.Implementation.Tests is the correct place,

Thanks, I've seen that the dependency relationship between "Jellyfin.Server.Implementation" and "Emby.Server.Implementation", I'll update the PR

Aligned code base to review comments:
	Jellyfin.Server.Implementation.Tests is the correct place
@@ -72,7 +72,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Jellyfin.Networking.Tests",
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Jellyfin.Dlna.Tests", "tests\Jellyfin.Dlna.Tests\Jellyfin.Dlna.Tests.csproj", "{B8AE4B9D-E8D3-4B03-A95E-7FD8CECECC50}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Jellyfin.XbmcMetadata.Tests", "tests\Jellyfin.XbmcMetadata.Tests\Jellyfin.XbmcMetadata.Tests.csproj", "{30922383-D513-4F4D-B890-A940B57FA353}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Jellyfin.XbmcMetadata.Tests", "tests\Jellyfin.XbmcMetadata.Tests\Jellyfin.XbmcMetadata.Tests.csproj", "{30922383-D513-4F4D-B890-A940B57FA353}"

This comment has been minimized.

@crobibero

crobibero Feb 13, 2021
Member

Why did this project guid change?

Looks like it was copied from the DLNA tests project

This comment has been minimized.

@lbenini

lbenini Feb 14, 2021
Author

First I'll naturaly update the PR to restore the previous guid, but now the analisys that is quite a rabbithole.

So all the projects already use "correctly" the guid {9A19103F-16F7-4668-BE54-9A1E7A4F7556} (from now on "9A") as the project type guid
the exception being Jellyfin.XbmcMetadata.Tests that uses {FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} (from now on "FA") due to being the last project added (see below)

I put correctly between quotes because as commented by a member of the Dot plaftorm in the solution there should be only the legacy guid "FA" dotnet/project-system#1821 (comment)
for compatibility reason but apparently VS like to use both:
New projects are added with GUID "FA" but the next time the solution file is updated, the existing ones are changed to "9A".

Bonus point: VS for Mac does the opposite

dotnet/project-system#1821

dkanada and others added 8 commits Feb 14, 2021
rename the solution file
100% branch coverage for Emby.Naming
Don't enable case-insensitivity for json by default
Default to English metadata during the setup wizard.
Return the correct ClientTypeName to allow Enum Parse
Added dynamic unit tests to ensure all BaseItem concrete descend
Aligned code base to review comments:
	Jellyfin.Server.Implementation.Tests is the correct place
Restored the project type guid as by review
See dotnet/project-system#1821
@lbenini
Copy link
Author

@lbenini lbenini commented Feb 14, 2021

I have updated the PR with the latest changes, and committed after a rebase, I'm unsure on how you prefer to manage the rebase in between a PR, eventually I can scrap this and do a new one.

@cvium cvium added this to Active PRs in Release 10.7.0 via automation Mar 4, 2021
@cvium cvium removed the stable backport label Mar 4, 2021
@cvium cvium removed this from Active PRs in Release 10.7.0 Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants