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 3 commits
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
5 changes: 5 additions & 0 deletions sdk/core/Azure.Core.TestFramework/src/RecordSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ public bool IsEquivalent(RecordSession session, RecordMatcher matcher)
session.Entries.SequenceEqual(Entries, new EntryEquivalentComparer(matcher));
}

/// <summary>
/// Indicates whether the <see cref="RecordSession"/> has any <see cref="Entries"/> or <see cref="Variables"/>.
/// </summary>
public bool IsEmpty => Entries.Count == 0 && Variables.Count == 0;

private class EntryEquivalentComparer : IEqualityComparer<RecordEntry>
{
private readonly RecordMatcher _matcher;
Expand Down
56 changes: 36 additions & 20 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 @@ -31,7 +31,7 @@ public TestRecording(RecordedTestMode mode, string sessionFile, RecordedTestSani
switch (Mode)
{
case RecordedTestMode.Record:
_session = new RecordSession();
session = new RecordSession();
if (File.Exists(_sessionFile))
{
try
Expand All @@ -47,11 +47,11 @@ public TestRecording(RecordedTestMode mode, string sessionFile, RecordedTestSani
case RecordedTestMode.Playback:
try
{
_session = Load();
session = Load();
}
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
christothes marked this conversation as resolved.
Show resolved Hide resolved
{
get
{
return _sessionInternal ?? _mismatchException switch
christothes marked this conversation as resolved.
Show resolved Hide resolved
{
null => _sessionInternal,
_ => throw _mismatchException
};
}
set
{
_sessionInternal = value;
}
}

private readonly RecordSession _session;
private readonly TestRecordingMismatchException _mismatchException;

private RecordSession _previousSession;

Expand Down Expand Up @@ -97,7 +113,7 @@ public TestRandom Random
_random = new TestRandom(Mode);
seed = _random.Next();
}
_session.Variables[RandomSeedVariableKey] = seed.ToString();
session.Variables[RandomSeedVariableKey] = seed.ToString();
_random = new TestRandom(Mode, seed);
break;
case RecordedTestMode.Playback:
Expand All @@ -108,7 +124,7 @@ public TestRandom Random
}
else
{
_random = new TestRandom(Mode, int.Parse(_session.Variables[RandomSeedVariableKey]));
_random = new TestRandom(Mode, int.Parse(session.Variables[RandomSeedVariableKey]));
}
break;
default:
Expand Down Expand Up @@ -144,10 +160,10 @@ public DateTimeOffset Now
// a number of auth mechanisms are time sensitive and will require
// values in the present when re-recording
_now = DateTimeOffset.Now;
_session.Variables[DateTimeOffsetNowVariableKey] = _now.Value.ToString("O"); // Use the "Round-Trip Format"
session.Variables[DateTimeOffsetNowVariableKey] = _now.Value.ToString("O"); // Use the "Round-Trip Format"
break;
case RecordedTestMode.Playback:
_now = DateTimeOffset.Parse(_session.Variables[DateTimeOffsetNowVariableKey]);
_now = DateTimeOffset.Parse(session.Variables[DateTimeOffsetNowVariableKey]);
break;
default:
throw new ArgumentOutOfRangeException();
Expand All @@ -172,13 +188,13 @@ private RecordSession Load()

public void Dispose(bool save)
{
if (Mode == RecordedTestMode.Record && save)
if (Mode == RecordedTestMode.Record && save && !session.IsEmpty)
{
var directory = Path.GetDirectoryName(_sessionFile);
Directory.CreateDirectory(directory);

_session.Sanitize(_sanitizer);
if (_session.IsEquivalent(_previousSession, _matcher))
session.Sanitize(_sanitizer);
if (session.IsEquivalent(_previousSession, _matcher))
{
return;
}
Expand All @@ -188,7 +204,7 @@ public void Dispose(bool save)
{
Indented = true
});
_session.Serialize(utf8JsonWriter);
session.Serialize(utf8JsonWriter);
utf8JsonWriter.Flush();
}
}
Expand All @@ -203,8 +219,8 @@ public HttpPipelineTransport CreateTransport(HttpPipelineTransport currentTransp
return Mode switch
{
RecordedTestMode.Live => currentTransport,
RecordedTestMode.Record => new RecordTransport(_session, currentTransport, entry => _disableRecording.Value, Random),
RecordedTestMode.Playback => new PlaybackTransport(_session, _matcher, _sanitizer, Random,
RecordedTestMode.Record => new RecordTransport(session, currentTransport, entry => _disableRecording.Value, Random),
RecordedTestMode.Playback => new PlaybackTransport(session, _matcher, _sanitizer, Random,
entry => _disableRecording.Value == EntryRecordModel.RecordWithoutRequestBody),
_ => throw new ArgumentOutOfRangeException(nameof(Mode), Mode, null),
};
Expand Down Expand Up @@ -252,7 +268,7 @@ public string GenerateAssetName(string prefix, [CallerMemberName] string callerM
{
if (Mode == RecordedTestMode.Playback && IsTrack1SessionRecord())
{
return _session.Names[callerMethodName].Dequeue();
return session.Names[callerMethodName].Dequeue();
}
else
{
Expand All @@ -262,20 +278,20 @@ public string GenerateAssetName(string prefix, [CallerMemberName] string callerM

public bool IsTrack1SessionRecord()
{
return _session.Entries.FirstOrDefault()?.IsTrack1Recording ?? false;
return session.Entries.FirstOrDefault()?.IsTrack1Recording ?? false;
}

public string GetVariable(string variableName, string defaultValue)
{
switch (Mode)
{
case RecordedTestMode.Record:
_session.Variables[variableName] = defaultValue;
session.Variables[variableName] = defaultValue;
return defaultValue;
case RecordedTestMode.Live:
return defaultValue;
case RecordedTestMode.Playback:
_session.Variables.TryGetValue(variableName, out string value);
session.Variables.TryGetValue(variableName, out string value);
return value;
default:
throw new ArgumentOutOfRangeException();
Expand All @@ -287,7 +303,7 @@ public void SetVariable(string variableName, string value)
switch (Mode)
{
case RecordedTestMode.Record:
_session.Variables[variableName] = value;
session.Variables[variableName] = value;
break;
default:
break;
Expand All @@ -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.

Loading