Skip to content

Add thorough tests for partially supported NRBF types #13490

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented May 20, 2025

This PR adds comprehensive tests for Hashtable serialization handling in CoreNrbfSerializer to ensure we're properly handling instances with unsupported content and not losing data over fields we don't currently handle (like the comparer).

The tests include:

  1. Verification that CoreNrbfSerializer.IsFullySupportedType() correctly returns false for Hashtable types
  2. Tests that standard Hashtable objects can be properly serialized and deserialized
  3. Tests that Hashtable objects with custom comparers:
    • Can be serialized but not deserialized by CoreNrbfSerializer
    • Retain all their data including the comparer when roundtripped through serialization
  4. Tests that Hashtable objects with custom hash code providers:
    • Can be serialized but not deserialized by CoreNrbfSerializer
    • Preserve their hash code provider information when roundtripped

These tests are important to ensure that while we may not fully handle all Hashtable configurations through CoreNrbfSerializer, we at least don't lose data when these objects pass through serialization, maintaining compatibility with existing functionality.

Fixes #12927.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • pkgs.dev.azure.com
    • Triggering command: dotnet restore src/System.Private.Windows.Core/tests/System.Private.Windows.Core.Tests/System.Private.Windows.Core.Tests.csproj (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits May 20, 2025 20:37
…ypes

Co-authored-by: JeremyKuhne <8184940+JeremyKuhne@users.noreply.github.com>
Co-authored-by: JeremyKuhne <8184940+JeremyKuhne@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Add thorough tests for partially supported NRBF types Add thorough tests for partially supported NRBF types May 20, 2025
@Copilot Copilot AI requested a review from JeremyKuhne May 20, 2025 20:43
@dotnet-policy-service dotnet-policy-service bot added the draft draft PR label May 20, 2025
Copilot finished work on behalf of JeremyKuhne May 20, 2025 20:43
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not update the global.json.

@dotnet-policy-service dotnet-policy-service bot added the waiting-author-feedback The team requires more information from the author label May 20, 2025
Co-authored-by: JeremyKuhne <8184940+JeremyKuhne@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented May 20, 2025

Do not update the global.json.

I've reverted the changes to global.json in commit d182b49. The file is now restored to its original state with the correct SDK version.

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label May 20, 2025
Copilot finished work on behalf of JeremyKuhne May 20, 2025 20:58
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not compile. Please fix the noted errors.

{ new Hashtable(StringComparer.CurrentCulture) { { "key", "value" } }, false },

// Hashtable with hash code provider won't be deserialized by TryGetPrimitiveHashtable
{ new Hashtable(new CustomHashCodeProvider()) { { "key", "value" } }, false },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructor overload does not exist. Use the one that takes an IComparer as a second argument (pass null for the second argument). Fix other instances in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the constructor overload in commit 7973629 by using the correct constructor that takes an IComparer as a second argument with null passed for the instances where only the hash code provider was needed.

@dotnet-policy-service dotnet-policy-service bot added the waiting-author-feedback The team requires more information from the author label May 20, 2025
Co-authored-by: JeremyKuhne <8184940+JeremyKuhne@users.noreply.github.com>
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label May 20, 2025
Copy link
Contributor Author

Copilot AI commented May 20, 2025

This code does not compile. Please fix the noted errors.

Fixed the compilation issues in commit 7973629 by:

  1. Adding CS0618 suppression for the entire class with explanatory comment
  2. Using the correct Hashtable constructor overload that takes both IHashCodeProvider and IComparer (passing null when only the provider is needed)

Copilot finished work on behalf of JeremyKuhne May 20, 2025 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add thorough tests for partially supported NRBF types
2 participants