delay throw of TestRecordingMismatchException until _session is accessed#18944
Conversation
christothes
requested review from
KrzysztofCwalina and
pakrym
as code owners
| catch (Exception ex) when (ex is FileNotFoundException || ex is DirectoryNotFoundException) | ||
| { | ||
| throw new TestRecordingMismatchException(ex.Message, ex); | ||
| _mismatchException = new TestRecordingMismatchException(ex.Message, ex); |
There was a problem hiding this comment.
Should we call load in the Session getter instead?
There was a problem hiding this comment.
hmm, actually there is one scenario where this doesn't work. HasRequests needs to not trigger Load but also be accurate after the ctor runs. This change would make that tricky.
There was a problem hiding this comment.
HasRequests is only used during recording so it might be okayish not to trigger load in it are return false if nothing is loaded.
There was a problem hiding this comment.
Although it happens to be true today, I'd be concerned that it could cause an issue if anyone tried to rely on HasRequests for any other reason.
There was a problem hiding this comment.
But in the current implementation, it also returns the wrong value when there is no Session loaded. In theory, it should throw.
There was a problem hiding this comment.
Isn't false still correct in that case? Perhaps we should add an additional SessionLoaded bool so that one could get the full picture if that is what is intended? I'm not sure that having to wrap HasRequests in a try/catch is better.
There was a problem hiding this comment.
Yeah, maybe. I just don't like relying on catching and suppressing exceptions.
Co-authored-by: Pavel Krymets <pavel@krymets.com>
Co-authored-by: Pavel Krymets <pavel@krymets.com>
2 participants
For scenarios where a recorded test wants to conditionally
Assert.Ignoreduring Setup, this deferral of throwingTestRecordingMismatchExceptionallows these tests to avoid having to create an empty test recording file.