Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

delay throw of TestRecordingMismatchException until _session is accessed #18944

Merged
merged 6 commits into from
Feb 24, 2021
Merged

delay throw of TestRecordingMismatchException until _session is accessed #18944

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e28c0d0
delay throw of TestRecordingMismatchException until _session is accessed
christothes Feb 23, 2021
e1c0b69
pr comments
christothes Feb 23, 2021
5c36ba0
use IsEmpty
christothes Feb 23, 2021
474eec2
Update sdk/core/Azure.Core.TestFramework/src/TestRecording.cs
christothes Feb 24, 2021
e3489ae
Update sdk/core/Azure.Core.TestFramework/src/TestRecording.cs
christothes Feb 24, 2021
c5d48b5
fix renaming error
christothes Feb 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions sdk/core/Azure.Core.TestFramework/src/TestRecording.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public TestRecording(RecordedTestMode mode, string sessionFile, RecordedTestSani
}
catch (Exception ex) when (ex is FileNotFoundException || ex is DirectoryNotFoundException)
{
throw new TestRecordingMismatchException(ex.Message, ex);
_mismatchException = new TestRecordingMismatchException(ex.Message, ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call load in the Session getter instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HasRequests is only used during recording so it might be okayish not to trigger load in it are return false if nothing is loaded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in the current implementation, it also returns the wrong value when there is no Session loaded. In theory, it should throw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe. I just don't like relying on catching and suppressing exceptions.

}
break;
}
Expand All @@ -66,8 +66,24 @@ public TestRecording(RecordedTestMode mode, string sessionFile, RecordedTestSani
private readonly RecordedTestSanitizer _sanitizer;

private readonly RecordMatcher _matcher;
private RecordSession _sessionInternal;
private RecordSession _session
{
get
{
return _sessionInternal ?? _mismatchException switch
{
null => _sessionInternal,
_ => throw _mismatchException
};
}
set
{
_sessionInternal = value;
}
}

private readonly RecordSession _session;
private readonly TestRecordingMismatchException _mismatchException;

private RecordSession _previousSession;

Expand Down Expand Up @@ -172,7 +188,7 @@ private RecordSession Load()

public void Dispose(bool save)
{
if (Mode == RecordedTestMode.Record && save)
if (Mode == RecordedTestMode.Record && save && _session.Entries.Count > 0 && _session.Variables.Count > 0)
{
var directory = Path.GetDirectoryName(_sessionFile);
Directory.CreateDirectory(directory);
Expand Down Expand Up @@ -299,7 +315,7 @@ public void DisableIdReuse()
_previousSession = null;
}

public bool HasRequests => _session?.Entries.Count > 0;
public bool HasRequests => _sessionInternal?.Entries.Count > 0;

public DisableRecordingScope DisableRecording()
{
Expand Down
342 changes: 0 additions & 342 deletions .../tests/SessionRecords/TableClientLiveTests(CosmosTable)/BatchInsertAndMergeAndDelete.json
View file
Open in desktop

This file was deleted.

342 changes: 0 additions & 342 deletions ...s/SessionRecords/TableClientLiveTests(CosmosTable)/BatchInsertAndMergeAndDeleteAsync.json
View file
Open in desktop

This file was deleted.

276 changes: 0 additions & 276 deletions .../SessionRecords/TableClientLiveTests(CosmosTable)/CustomEntityMergeRespectsEtagAsync.json
View file
Open in desktop

This file was deleted.

Loading