Move version check earlier #6674
Conversation
| // For the latter case, internals may be unexpectedly null. | ||
| if (retVal == null || version != CurrentSerializationVersion) | ||
| { | ||
| log.LogMessageFromResources("General.CouldNotReadStateFileMessage", stateFile, log.FormatResourceString("General.IncompatibleStateFileType")); |
There was a problem hiding this comment.
I think it continues to make sense for this to be a quiet message. But do we have a test in place that this is not the case for freshly-built-everything using the SDK cache?
There was a problem hiding this comment.
We do not. If I remember correctly, we decided it would be too hard for some reason.
There was a problem hiding this comment.
Can we at least get it added to the manual SDK tests?
There was a problem hiding this comment.
Yeah, that sounds like a good idea to me. @marcpopMSFT, agree? If so, I can write up some (very detailed) instructions on testing it.
There was a problem hiding this comment.
Please reach out to Richa with the scenario you want covered. The difficulty with fresh machine tests as you can only have one.
src/Tasks/StateFileBase.cs
Outdated
| byte version = 0; | ||
| translator.Translate(ref version); | ||
| // If retVal is still null or the version is wrong, log a message not a warning. This could be a valid cache with the wrong version preventing correct deserialization. | ||
| // For the latter case, internals may be unexpectedly null. | ||
| if (retVal == null || version != CurrentSerializationVersion) |
There was a problem hiding this comment.
retval will always be null here, right?
| if (retVal == null || version != CurrentSerializationVersion) | |
| if (version != CurrentSerializationVersion) |
… into move-version-check
| // For the latter case, internals may be unexpectedly null. | ||
| if (retVal == null || version != CurrentSerializationVersion) | ||
| { | ||
| log.LogMessageFromResources("General.CouldNotReadStateFileMessage", stateFile, log.FormatResourceString("General.IncompatibleStateFileType")); |
There was a problem hiding this comment.
Can we at least get it added to the manual SDK tests?
src/Tasks/StateFileBase.cs
Outdated
| // If retVal is still null or the version is wrong, log a message not a warning. This could be a valid cache with the wrong version preventing correct deserialization. | ||
| // For the latter case, internals may be unexpectedly null. |
I still need to write instructions. Bit behind on that front. |
Forgind
added
the
merge-when-branch-open
Fixes #6659
The version check was previously after having deserialized everything else. This doesn't make sense and can lead to errors. In this PR, I moved it up.