Skip to content

Add ScrubNumericIds#1704

Merged
SimonCropp merged 8 commits into
mainfrom
ScrubNumericIds
Apr 3, 2026
Merged

Add ScrubNumericIds#1704
SimonCropp merged 8 commits into
mainfrom
ScrubNumericIds

Conversation

@SimonCropp
Copy link
Copy Markdown
Member

No description provided.

@SimonCropp SimonCropp added this to the 31.15.0 milestone Mar 31, 2026
@SimonCropp SimonCropp changed the title ScrubNumericIds Add ScrubNumericIds Mar 31, 2026
@fiseni
Copy link
Copy Markdown
Contributor

fiseni commented Apr 1, 2026

The Id scrubbing unfortunately turned out to be a bit more peculiar. Unlike Guid/Date which are globally unique, auto incrementing database Ids are a bit weird.

Let me describe the setup and the problem.

  • If you create the database per each test, then the current approach works and the output is stable.
  • If you create the database per collection fixture or even class fixture, then:
    • If you run the tests for the whole collection, it's still deterministic and stable. You may re-run them and will succeed.
    • Once you have accepted the snapshots, if you run a specific test only, then you may get different results.
    • The opposite is true as well.

Sample

public class VerifySampleTests
{
    [Fact]
    public async Task Test1()
    {
        var customers = new List<Customer>
        {
            new() {
                Id = 1,
                Addresses =
                [
                    new() { Id = 1, CustomerId = 1 },
                    new() { Id = 2, CustomerId = 1 }
                ]
            },
            new() {
                Id = 2,
            }
        };
        await Verify(customers);
    }


    class Address
    {
        public int Id { get; set; }
        public int CustomerId { get; set; }
    }

    class Customer
    {
        public int Id { get; set; }
        public List<Address> Addresses { get; set; } = [];
    }
}

Imagine the customers are fetched from DB. If you run this test in isolation, this will output:

[
  {
    Id: Id_1,
    Addresses: [
      {
        Id: Id_1,
        CustomerId: Id_1
      },
      {
        Id: Id_2,
        CustomerId: Id_1
      }
    ]
  },
  {
    Id: Id_2
  }
]

If you run the whole test suite, the second customer for instance may have Id = 3 (depending what other tests have inserted), and will result to the following output.

[
  {
    Id: Id_1,
    Addresses: [
      {
        Id: Id_1,
        CustomerId: Id_1
      },
      {
        Id: Id_2,
        CustomerId: Id_1
      }
    ]
  },
  {
    Id: Id_3
  }
]

I created this gist. It's not a sophisticated approach, meddling into the serializer. But, this will always be stable and the output will be as follows (regardless of what Id has the second customer). It's scoped per model. Yes, in this case, the counter for Id_ is different for Customer and Address, but.. not important?

https://gist.github.com/fiseni/aa78e0b6c3e51d41e0683e227ddc297c

[
  {
    Id: Id_1,
    Addresses: [
      {
        Id: Id_1,
        CustomerId: CustomerId_1
      },
      {
        Id: Id_2,
        CustomerId: CustomerId_1
      }
    ]
  },
  {
    Id: Id_2
  }
]

PS. Perhaps I have more specific use-case. Folks may find this useful as it is.

EDIT: For context, after each test ofc I restore the db state to a predefined state. I can perhaps reseed the identity id in this process, and that would solve these inconsistencies.

@SimonCropp
Copy link
Copy Markdown
Member Author

@fiseni i took another pass at this. can u review again?

@fiseni
Copy link
Copy Markdown
Contributor

fiseni commented Apr 3, 2026

Lovely! Do you have some alpha channel? Once you release it, I can test it more thoroughly. But as far as I can see it looks great now.

PS. I was thinking about the last sentence in my previous comment. If I make the outputs deterministic, then I wouldn't need scrubber at all. So, this feature indeed saves me from the hassle. Thank you!

@SimonCropp SimonCropp modified the milestones: 31.15.0, 31.16.0 Apr 3, 2026
@SimonCropp SimonCropp merged commit 831dbca into main Apr 3, 2026
5 of 7 checks passed
@SimonCropp SimonCropp deleted the ScrubNumericIds branch April 3, 2026 10:06
@fiseni
Copy link
Copy Markdown
Contributor

fiseni commented Apr 4, 2026

@SimonCropp

I tested 31.16.0-beta.1 more thoroughly. The output is stable. I have two small notes:

  • The snippets in the docs are missing. Perhaps some job failed.
  • The default behavior, I think, is that if the properties have default values you're not serializing them at all to the output. With this feature enabled, all Id properties are serialized regardless if they have default values (e.g. Customer.Id = 0 to Customer_Id1, CustomerId = 0 to CustomerId_1, Address.CountryId = null to CountryId_null, etc). I don't mind this, but not sure if it will break existing tests for users. It's worth noting it.

@SimonCropp
Copy link
Copy Markdown
Member Author

@fiseni

The snippets in the docs are missing. Perhaps some job failed.

sorry. that is the doc template. the produced doc is https://github.com/VerifyTests/Verify/blob/main/docs/numeric-ids.md

@fiseni
Copy link
Copy Markdown
Contributor

fiseni commented Apr 6, 2026

Ahh my bad

@SimonCropp
Copy link
Copy Markdown
Member Author

@fiseni no need to apologise.

The default behavior, I think, is that if the properties have default values you're not serializing them at all to the output. With this feature enabled, all Id properties are serialized regardless if they have default values (e.g. Customer.Id = 0 to Customer_Id1, CustomerId = 0 to CustomerId_1, Address.CountryId = null to CountryId_null, etc). I don't mind this, but not sure if it will break existing tests for users. It's worth noting it.

i think this is only if people opt in to ScrubNumericIds, so not an issue?

@fiseni
Copy link
Copy Markdown
Contributor

fiseni commented Apr 6, 2026

Yes, that's true. But imagine folks may have a lot of dto models (not only entities) with Id properties. All current snapshots won't have them serialized if they have default values. Once they add the scrubber, they may get many failing tests (which may be confusing if they weren't expecting them there).

I mean it's not a problem at all. They should just accept all new snapshots. Sometimes I'm just over cautious :)

agocke pushed a commit to serdedotnet/serde that referenced this pull request May 16, 2026
Updated [Verify.XunitV3](https://github.com/VerifyTests/Verify) from
31.15.0 to 31.16.3.

<details>
<summary>Release notes</summary>

_Sourced from [Verify.XunitV3's
releases](https://github.com/VerifyTests/Verify/releases)._

## 31.16.3

No issues in this milestone yet.

## 31.16.2

- [x] [#​1714](VerifyTests/Verify#1714)
Unexpected empty.nupkg in output directory
- [x] [#​1721](VerifyTests/Verify#1721) XUnit
V3: Adds attachment with colon in filename - GitHub artifacts upload
fails
- [x] [#​1723](VerifyTests/Verify#1723) Strip
solution dir prefix from XunitV3 attachment names

## 31.16.1

- [x] [#​1718](VerifyTests/Verify#1718) Handle
eof and missing ihdr in png decoder

## 31.16.0

- [x] [#​1704](VerifyTests/Verify#1704) Add
ScrubNumericIds
- [x] [#​1707](VerifyTests/Verify#1707) Minor
micro-optimizations for Verify project.
- [x] [#​1708](VerifyTests/Verify#1708) Add
Target comparer and avoid string allocations.
- [x] [#​1709](VerifyTests/Verify#1709)
ApplyForPropertyValue returns string instead of CharSpan
- [x] [#​1710](VerifyTests/Verify#1710) In-place
sort instead of LINQ OrderByDescending
- [x] [#​1711](VerifyTests/Verify#1711) Remove
unnecessary ToString() in VerifyJsonWriter
- [x] [#​1716](VerifyTests/Verify#1716) add Png
ssim comparer
- [x] [#​1717](VerifyTests/Verify#1717) Throw on
null assembly in TryGetType for bad plugin assembly

Commits viewable in [compare
view](VerifyTests/Verify@31.15.0...31.16.3).
</details>

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=Verify.XunitV3&package-manager=nuget&previous-version=31.15.0&new-version=31.16.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants