Prep tests for CoreCLR/CoreRT [Serializable] cleanup #20035
Conversation
…alizable that won't be serializable soon.
|
CC @dotnet/corert-contrib |
| public void Serialization() | ||
| { | ||
| const string message = "FATAL ERROR: The pizza could not be found."; | ||
| var ex = new MissingManifestResourceException(message); |
There was a problem hiding this comment.
Serializatoin works, right - through the base Exceptoin type. Which probably has all the fields anyway. It just does not deserialize to the expected derived type.
There was a problem hiding this comment.
No. If type B derives from A and B isn't marked [Serializable], it doesn't matter whether A is; B can't be serialized with BinaryFormatter.
| { | ||
| var ex = new COMException("E_BAD_PIZZA", -1337); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } |
| { | ||
| var ex = new InvalidComObjectException("E_BAD_PIZZA"); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } |
| { | ||
| var ex = new InvalidOleVariantTypeException("E_BAD_PIZZA"); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } |
| { | ||
| var ex = new MarshalDirectiveException("E_BAD_PIZZA"); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } |
| { | ||
| var ex = new SEHException("E_BAD_PIZZA"); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } |
| { | ||
| var ex = new SafeArrayRankMismatchException("E_BAD_PIZZA"); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } |
| { | ||
| var ex = new SafeArrayTypeMismatchException("E_BAD_PIZZA"); | ||
| BinaryFormatterHelpers.AssertRoundtrips(ex); | ||
| } | ||
| } |
|
@morganbr this doesn't look like 2.0 work, moving to 2.1. It is against master branch, so this particular PR will be in 2.1 anyway. |
|
Ensuring we ship the right serialization is definitely 2.0. |
|
@stephentoub in that case we need another PR. This PR is against master, so it will go o its own only into 2.1 -- that's what the milestone should IMO track. Not the intent of other PRs, but the fact where this PR truly lands. |
|
Why have milestones on PRs at all then? After the fact you can just see where the commits are. |
|
@stephentoub I am happy to chat, but the key thing is that I don't know how to get that information as quickly as just look at PR milestone. |
|
Created porting bug #20041 to track the port to 2.0 |
|
@karelz, if you're saying that we'll change the milestone on this PR to 2.0 once it's ported to release/2.0 and you simply don't want it to be changed before then, that's fine. If you're saying this PR would remain marked 2.1 and the porting PR would be marked 2.0, that doesn't make sense to me. |
|
@stephentoub ok, now I am very interested. Obviously you think about milestones in entirely different way than I do and I was convinced that the way I look at them is the only way / natural way, which means I am "blind" and I am missing some angles. I would love to learn more / chat. Here's my take / motivation (not trying to convince you, just explaining how I look at it): Now why all this? I am curious what's your view point. What I don't see? |
@karelz, consider this PR: #19997. Let's say it's a year from now. If the purpose of setting a milestone on a PR is to be able to quickly determine whether a given code change shipped in a given release, did that fix ship in 2.0? If you have to look at the thread to understand it, then what's the purpose of the milestone metadata, and if you don't look at the thread, then just looking at the milestone is going to give you the wrong answer; it says 2.1.0, but this fix is in 2.0.0, it was just a different PR that put it there. |
|
Well, first line of that PR kind of suggests to look at the issue (which is 2.0 milestone) and it also mentions it is same as something in rel/2.0.0, which gives a hint this might be fixed in 2.0. BTW: Also, don't view this as perfect system, but IMO 99% things are correct. I spent some time back in November-January to fixup milestones of 1.0 and 1.1 issues & PRs. I bet I made a few mistakes exactly around forking times, especially when we had 2 bugs filed for 2 releases, without proper linking. The good news is, we can always fix it in future when particular questions are asked. Overall having the milestones is IMO slightly better than not having them at all (i.e. PRs have always empty milestone), or having them used randomly. I am willing to spend the time on the updates (my OCD kicks in ;)) and if it helps just 1 person per quarter, I view it as reasonable investment. I know for sure that I can rely on the info when someone asks me "is X fixed in 2.0"? |
Then why stop where you're stopping? What is the downside to changing the milestone on a PR like the one I linked to 2.0? From my perspective, either the milestone should reflect the lowest version that contains that fix, or it's misleading. I don't see any gray area there. |
|
There are 3 cases:
In case (1) when there is only PR. Let's say you mark it as 2.0. It gets merged. Who is responsible for creating the PR against the other branch (rel/2.0.0, but it might be the other way)? |
We're talking past each other. I'm not saying mark it as 2.0 immediately when the PR is created, I'm saying mark it as 2.0 after it's in 2.0. |
|
And to answer your question: Yes, I agree with your statement:
But only in case (2) - when there is single issue tracking the fix and only for the issue. PRs (case (1)) are different as I tried to explain above. |
|
The system simply does not make sense to me. The way this is set up, I can't trust milestones on PRs to mean anything meaningful; a PR might say 2.1 but the fix is actually already in 2.0. Either it shouldn't be set, or it should be accurate. I'll leave it at that. |
The only way to be sure is to check the history in release/2.0 branch. There are many opportunities for mistakes.
There is issue for this. It just was not linked with this PR. Anything important has an issue. I do not think that it is interesting to worry about the problem of PRs without an issue.
... and potentially multiple PRs in same branch, or multiple PRs in various repos. We have been on the single issue pointing to multiple PRs plan for regular development and for ask mode, and some of the time for servicing as well (when the fix in master is done at the same time as the servicing fix).
Right. The right way to do that is to ensure that any fix that goes to anywhere but master has the uber tracking issue linked to it. |
I'm happy to chat instead of typing ...
I see, that was not clear to me. Sure, in theory why not, I probably don't care about that nuance, however it is quite a lot of additional work which I am personally not willing to do (not the right cost/benefit for me). Current system is easy to do for me via GH queries and bulk-edits (couple of minutes per day).
Sure. First I don't know how to do that. Git tooling is quite confusing for me in that area (maybe I am just dummy). Second I am 99% sure majority of our users won't do that - they will rather ask. If they get the answer (99% accurate) from the milestone, everyone's life is easier (especially mine).
Well, I didn't know that. As we see, even experienced engineers like Morgan sometimes forget to link things :( ... so I assume we will have occasional cases like this one. |
Fixes tests that depend on types in System.Private.CoreLib being serializable that won't be serializable soon.
Fixes tests that depend on types in System.Private.CoreLib being serializable that won't be serializable soon.
Fixes tests that depend on types in System.Private.CoreLib being serializable that won't be serializable soon.
Fixes tests that depend on types in System.Private.CoreLib being serializable that won't be [Serializable] soon in CoreCLR/CoreRT.
Part of https://github.com/dotnet/corefx/issues/19119