GeezerVison

I'm not sure if the scenario is supported, but does Update-TypeData and Remove-TypeData work with shared type tables? And if so, will that scenario continue to work with these changes?

Update-TypeData and Remove-TypeData work directly on TypeTable and nothing has changed there and the TypeTables remain the same. This bug is when passing the TypeTable to the ISS so I think it is Ok. But I am running regression tests to ensure nothing is broken.

I would be surprised if we have tests covering the scenario - I think you'll need to write some new tests.

As near as I can tell these cmdlets have nothing to do with the bug I fixed. They affect the TypeTable directly (add/remove) but are not involved with ISS binding to a runspace where the fix is. Maybe I am missing something but I don't see a scenario that needs testing.

But the TypeTable is shared. If one changes the TypeTable in one runspace, then uses that shared TypeTable to construct another runspace, does your code handle that correctly?

I think this is a non-issue because changing a shared type table should not be allowed, but I was asking that you confirm that, preferably with tests because I wasn't confident your code was correct if it was allowed.

Ok, I understand you now. I agree that this is a non-issue, not necessarily because changing a shared type table is not allowed but because the TypeTable is consistent in how it stores types. But I'll add a test for this.

I verified that Update-TypeData and Remove-TypeData does not affect ISS TypeTable. I also verified that the code disallows a shared TypeTable in the ISS (unless it is the only entry), and added a test for this.

Your check at the start of the function means you can cast instead of using as, which you should prefer because you're not testing if typeEntry is null. And I suppose we don't allow adding null entries?

I thought so too but a direct cast isn't allowed:

var typeEntry = (SessionStateTypeEntry)element;
"Cannot covert type T to SessionStateTypeEntry"
It looks like a runtime cast is needed. Or is there another way?

I don't know why a cast is not allowed - T is even constrained to be InitialSessionStateEntry, so C# should be smart enough to allow the cast. Oh well.

I also tried using a type constrained on the method but that failed as well because SessionStateTypeEntry is a sealed class which is disallowed as a constraint...

How about { $rs2.Open() } | Should Not Throw

Thanks. Much better

As suggested by our test guideline, maybe the following is better:

try
{
   $rs.Open()
   throw "No Exception!"
}
catch
{
    $_.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should Be "ErrorsUpdatingTypes"
}

I have already been through this with Illya. The problem with this is that this can result in a null reference exception. It is better to test whether the desired error Id is found.

There is no method call or indexing operation involved, only property access, so it shouldn't trigger a null reference exception. Can you please explain where the exception might be raised?

try { throw "No Exception!" } catch { $_.Exception.InnerException -eq $null }

property access on null value doesn't raise exception unless it's in strict mode and our test doesn't run in strict mode. But it's fine to be cautious.

Good point, but I am never sure how Pester will run a block, for example ErrorActionStop. I guess I am good with either way.

Maybe the following check is more readable?

if (typeof(T) != typeof(SessionStateTypeEntry))
{
    throw new PSInvalidOperationException();
}

I don't think it is more readable, but it does seem simpler than checking for a valid cast/conversion.