All of these are redundant, and we already don't explicitly map all the other cases of NpgsqlDbType either.

I think the intent here was mostly just to be explicit in the source code. No issue with removing.

No exactly, I'm fine either way too but then we should just fill it out.

For example this comes back as DbType.Object for value based inference because Interval is the first mapping for TimeSpan, which has no DbType mapping.

This on the other hand returns DbType.Object when we do know the data type, as time with time zone has no sensible DbType case.

All of these nodatime mappings specify DbType.Object for the value based inference, as these nodatime types are unknown to npgsql before parameter binding.

These byte[], ROM and stream tests all default to DbType.Binary through value inference due to them being bytea defaults, which maps to DbType.Binary.

This is one of the few places we use the three parameter constructor. (around datetime there are another few cases)

We use it when we want to check whether the alias cases write correctly (e.g. varnumeric is an alias of decimal for us, but we want to know whether that mapping works).

In this case the first argument is the db type to set on the parameter, the second and third arguments are the same as the two value constructor (first comes the datatype based dbtype, then the value inferred dbtype).

In this assert we expect it to return DbType.Decimal for both. DataTypeName "numeric" being mapped to DbType.Decimal and CLR Type decimal being mapped to DbType.Decimal as well.

I think the intent here was mostly just to be explicit in the source code. No issue with removing.

Tiny nit: when we split arguments to different lines as in this case, I generally prefer to do all of them on different lines - I personally find it easier to read. Obviously no big deal.

Yeah in most of these cases that I changed I have paired them with their related arguments. That's mostly why they come like that. I personally find it scans a bit more easily if I can scan a concept per line. Completely fine to change this.

I have to say that I find the current form more immediately understandable and useful as a "spec/documentation" of Npgsql's behavior - this is clearly telling me that PG timestamp does not get read as LocalDateTime by default (i.e. one must do GetFieldValue<LocalDateTime>() to explicitly request it), whereas LocalDateTime does get written by default as PG timestamp (i.e. one doesn't need to set DataTypeName/NpgsqlDbType).

On the new side of the diff I now see dataTypeInference I'm not longer seeing that information. It could be just my way of thinking (I wrote the current system after all) but it seems like some information is getting lost here no?

Note that I definitely am for tightening assertions and checks, renaming things and other stuff you're doing in this PR - just not sure if this specific change to the actual AssertType API is necessary for that etc.

I see, yes I think indeed that's a small difference in understanding. To me GetValue() is not necessarily a default (it also has to be called) or for that matter any less explicit than a generic method. That also fits with the reasoning around GetFieldValue<object> needing to do the same thing as GetValue(). But I agree that if you want something other than a boxed object instance of *some* type, you do have to know about that type and pass it. For those cases there is indeed more information requested of the caller, if that's what you mean.

whereas LocalDateTime does get written by default as PG timestamp (i.e. one doesn't need to set DataTypeName/NpgsqlDbType).

This part is mostly you inferring this from your existing experience, as we're not really passing the write side default argument at all. Though I agree that currently there is a nice (looking) 'default' parameter for writing and reading.

The most significant point of contention I have with “default” is on the write side though. There it really doesn't work well conceptually due to the multiple ways we try to attempt to find a mapping if we have no data type to go on (where we check whether we have an actual default mapping, but also attempt to fallback to the first clr type match if we don't have a default mapping). Which also means that if you just go by seeing whether a mapping does not have isDefault:true (in a type info resolver) that it does not mean it won't be seen as a 'default' for writing in the current tests. Unless you know exactly what all this entails you will just try some flags until the test passes, maybe having disabled half the checks (which is why I'm adding more negative matching too).

Regarding the isDefaultForReading name change (or just isDefault for AssertTypeRead) to isValueTypeDefaultFieldType. Given that we *always* align the type coming from GetValue() and FieldType (this was also true in the handler system) I elaborated the name to add the mention of its input valueType (typeof(T) basically), and the output it checks FieldType.

Now in the PR I mentioned that I wanted to get rid of the notion of default in the names of things, and on (re)reviewing this myself I can't say I have succeeded 😅, so the new name leaves a lot to be desired. I have pushed a change to rename this to valueTypeEqualsFieldType instead (also note that it's not used a lot either way, about 100 hits where actual tests pass this in).

I hope that valueTypeEqualsFieldType is something you (and @vonzshik) can get used to, but if you feel strongly about the name or the approach we should discuss it a bit.

The trend for this PR should be to have assertion argument names better describe which inputs are being checked against which outputs, and to drive down the number of (argument level) knobs. The fewer we can get away with the more we can confidently say we have a predictable and consistent mapping system, which also helps future code align better to those standards.

So I think we're we might have a gap is around looking at the type mapping system from the inside or from the outside... I'm generally looking at these tests 100% from a user perspective - I generally like that tests encode the user contract as much as possible (where that makes sense). And if you look at our type mapping docs, you'll see the same "default" concept expressed in the two tables:

• On the read side (1st table), each PG type has a single .NET read type, e.g. PG text gets read as .NET string by the untyped APIs (e.g. GetValue()), but can also be read as char[] when the typed API is used (GetFieldValue<T>()).
• In my head (and currently in the docs - 2nd table) the same kinda logic works on the write side: each .NET type has a single PG type, which is what Npgsql writes unless you tell it to write something else via NpgsqlDbType/DbType/DataTypeName.

To me both of the above map pretty nicely to the concept of "default" - from a very user-oriented perspective - are you saying you think it's problematic? If so, would you explain it in another way to users in the docs, which would simplify things somehow? Or can you give a problematic scenario or two where describing things to users as "default" (either on the read or write side) does a disservice/is incorrect? Because if thinking about this as "default" works for explaining our behavior to users, it feels like it should work in the tests too (though I get that this might not always be 100% true).

Regarding the isDefaultForReading name change (or just isDefault for AssertTypeRead) to isValueTypeDefaultFieldType. Given that we always align the type coming from GetValue() and FieldType (this was also true in the handler system) I elaborated the name to add the mention of its input valueType (typeof(T) basically), and the output it checks FieldType.

Sure, I understand - in my head the alignment of GetValue() and FieldType just naturally flows from, well, having a default .NET type that's read for the given PG type (i.e. since PG text is by default read as string, GetValue() and FieldType always return string). So your proposed naming just seems to go more low-level, encompass two things and therefore be a bit less immediately understandable, where there's (for me) a simple high-level concept that already expressed this. But I can't say it bothers me that much, if it's important to you.

BTW I'm posting a long/serious answer here because it's an interesting discussion, but it's not like I have super-strong feelings about it either :) This is just test factoring after all.

Maybe document these? I honestly am not sure what exactly they mean in the context of these tests and need to read the code to figure it out.

Yes, will do. I'll also add a bit of rationale here about the what and why of dataTypeInference as the assertion argument.

Took a shot at letting opus generate a rationale.

Please explain what dataTypeInference in AssertType does

Opus:

DataTypeInference controls how the test validates Npgsql's ability to infer the PostgreSQL data type from a CLR value.

What it tests:

• Exact (or true): The CLR type uniquely maps to the expected PostgreSQL type. The test verifies that setting only the Value property (without NpgsqlDbType or DataTypeName) correctly infers the exact type. Both generic and non-generic parameters are tested.

• WellKnown: The CLR type maps to some PostgreSQL type (not Unknown), but not necessarily the expected one. Used when a type is recognized but ambiguous—the test accepts whatever type Npgsql infers rather than requiring an exact match.

• Unknown (or false): The CLR type cannot be used to infer any PostgreSQL type. The test expects DataTypeName to be null and NpgsqlDbType to be Unknown. Value-only parameters are excluded from the roundtrip test since they'd fail.

In practice:

The test always validates explicit type specification (via NpgsqlDbType, DataTypeName, DbType). The DataTypeInference parameter determines whether value-only scenarios are tested and what inference behavior is expected.

Could you explain why WellKnown exists over having just Unknown without negative matching?

Opus:

The key difference is what gets tested vs skipped:

With WellKnown (current approach):

• Value-only parameters are still included in the roundtrip test
• The test writes the value and verifies it round-trips correctly
• It just doesn't assert the specific inferred type (accepts whatever Npgsql infers)

With only Unknown (no negative matching):

• Value-only parameters would be skipped entirely from the test
• You lose coverage that the value can be written at all via inference

Example: Testing string → json

The WellKnown approach catches regressions where a CLR type that used to infer successfully suddenly fails to infer anything. With Unknown, you'd never notice because that scenario isn't tested.

I'll add that this parameter just replaces isDataTypeInferredFromValue (it's a rename + the addition of the WellKnown to enable negative matching). However, as value only inference is really the only observable property for what we used to consider 'defaultness' it also meant isDefaultForWriting could be removed. Similarly to how valueTypeEqualsFieldType is all that is relevant for defaultness on the read side.