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.

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

OSOE-353: Add duplicate SQL query detector in Lombiq.UITestingToolbox #216

Open
wants to merge 70 commits into
base: dev
Choose a base branch
from
Open
Changes from 53 commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
f224aff
Adding counter infrastructure
dministro Oct 23, 2022
d87e325
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Oct 23, 2022
34b538b
Using interfaces
dministro Oct 24, 2022
7574e9a
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Oct 24, 2022
beef486
Implementing required exception constructors
dministro Oct 24, 2022
6c3602f
Adjusting tests
dministro Oct 25, 2022
06f8eaa
Fixing typo
dministro Oct 25, 2022
3cd03e3
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Oct 25, 2022
8335d59
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Nov 7, 2022
914c291
Update Lombiq.Tests.UI/Services/CounterDataCollector.cs
dministro Dec 3, 2022
1100237
Update Lombiq.Tests.UI/Services/Counters/CounterProbeBase.cs
dministro Dec 3, 2022
df3c3c2
Update Lombiq.Tests.UI/Services/Counters/ICounterValue.cs
dministro Dec 3, 2022
1a0fd1a
Update Lombiq.Tests.UI/Services/Counters/NavigationProbe.cs
dministro Dec 3, 2022
8e61c22
Update Lombiq.Tests.UI.Samples/Tests/BasicOrchardFeaturesTests.cs
dministro Dec 3, 2022
a20311c
Update Lombiq.Tests.UI/Services/CounterConfiguration.cs
dministro Dec 3, 2022
f033443
Merge branch 'issue/OSOE-353' of https://github.com/Lombiq/UI-Testing…
dministro Dec 3, 2022
cac8255
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Dec 3, 2022
169fa26
Adding SessionProbe
dministro Dec 3, 2022
cc941b0
Adding better exclusion and defaults
dministro Dec 4, 2022
8e854fc
Addressing "Add docs to the properties."
dministro Dec 6, 2022
dff34c3
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Dec 6, 2022
33fd4fe
Fixes after merge
dministro Dec 6, 2022
f0d8fc0
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Dec 15, 2022
0a8189c
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Dec 26, 2022
d0a088f
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Jan 13, 2023
5d32c2f
Fixing analyzer errors
dministro Jan 13, 2023
448a46e
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Jan 25, 2023
0e1cdfe
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Feb 13, 2023
eb8adc9
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Mar 22, 2023
b416eb5
Fixes after merge
dministro Mar 22, 2023
bc91080
Fixing possible thread safety violation
dministro Mar 24, 2023
b3e0600
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Mar 24, 2023
278fdcc
Addressing "S4457: Split this method into two..." analyzer error
dministro Mar 27, 2023
2fd58bd
Adding per url threshold configuration support
dministro Apr 3, 2023
98a596e
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro May 18, 2023
8c05e71
Adding per page configuration implementation
dministro May 19, 2023
097d091
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Jul 17, 2023
030c692
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Aug 27, 2023
406e6ed
Fixing analyzer violations
dministro Aug 27, 2023
6bf2917
Adding ProbedSqliteConnection
dministro Aug 27, 2023
acf440f
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Sep 3, 2023
3342c9d
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Sep 8, 2023
126c97b
Improving log and exception message
dministro Sep 10, 2023
82c9090
Improving logs
dministro Sep 10, 2023
f50c13c
Throwing session probe exception in test context
dministro Sep 23, 2023
8a0b33c
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Oct 1, 2023
de38ccd
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Oct 19, 2023
8561fac
Fixing test
dministro Oct 23, 2023
fb9be54
Adding comment
dministro Oct 23, 2023
4935a11
Fix spelling
dministro Oct 23, 2023
c2906c7
Apply suggestions from code review
dministro Nov 5, 2023
8c58730
Apply suggestions from code review
dministro Nov 12, 2023
c953c8c
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Jan 30, 2024
3343bd8
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Apr 9, 2024
4f334fe
Fixes after merge
dministro Apr 9, 2024
027486d
Fixing analyzer violations
dministro Apr 9, 2024
fa3a95c
Fixing analyzer violations again
dministro Apr 9, 2024
0e4f177
Adding custom type for storing db command parameters in configuration…
dministro May 6, 2024
849553f
Allowing to enable/disable counter subsystem by configuration
dministro May 7, 2024
9203e36
Adding a bit more information to CounterThresholdException message
dministro May 7, 2024
a52cff1
Removing unnecessary Should.NotThrowAsync()
dministro May 7, 2024
30b4997
Adding test to demonstrate the scenario when the counter thresholds a…
dministro May 7, 2024
2d5abae
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro May 7, 2024
1b500d8
Removing unnecessary configuration
dministro May 12, 2024
bca3add
Renaming `PageLoadProbe` -> `RequestProbe` and `PageLoadProbeMiddlewa…
dministro May 12, 2024
a1531d5
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro May 12, 2024
71663b9
Adding more comments
dministro May 13, 2024
1dea988
Merge branch 'dev' of https://github.com/Lombiq/UI-Testing-Toolbox in…
dministro Jul 14, 2024
2c7e741
Spelling
dministro Jul 14, 2024
a6f685c
Fixing analyzer warnings
dministro Jul 14, 2024
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
1 change: 1 addition & 0 deletions Lombiq.Tests.UI.Samples/Readme.md
Original file line number Diff line number Diff line change
@@ -29,6 +29,7 @@ For general details about and on using the Toolbox see the [root Readme](../Read
- [Testing in tenants](Tests/TenantTests.cs)
- [Interactive mode](Tests/InteractiveModeTests.cs)
- [Security scanning](Tests/SecurityScanningTests.cs)
- [Duplicated SQL query detector](Tests/DuplicatedSqlQueryDetectorTests.cs)

## Adding new tutorials

17 changes: 15 additions & 2 deletions Lombiq.Tests.UI.Samples/Tests/BasicOrchardFeaturesTests.cs
Original file line number Diff line number Diff line change
@@ -20,8 +20,21 @@ public BasicOrchardFeaturesTests(ITestOutputHelper testOutputHelper)
// We could reuse the previously specified SetupHelpers.RecipeId const here but it's actually a different recipe for
// this test.
[Fact]
public Task BasicOrchardFeaturesShouldWork() =>
ExecuteTestAsync(context => context.TestBasicOrchardFeaturesAsync(RecipeIds.BasicOrchardFeaturesTests));
public Task BasicOrchardFeaturesShouldWork(Browser browser) =>
ExecuteTestAsync(
context => context.TestBasicOrchardFeaturesAsync(RecipeIds.BasicOrchardFeaturesTests),

configuration =>
{
// The UI Testing Toolbox includes a DbCommand execution counter to check for duplicated SQL queries..
// After the end of the test, it checks the number of executed commands with the same SQL command text
// and parameter set against the threshold value in its configuration. If the executed command count is
// greater than the threshold, it raises a CounterThresholdException. So here we set the minimum
// required value to avoid it.
configuration.CounterConfiguration.Running.PhaseThreshold.DbCommandExecutionThreshold = 26;
Copy link
Member

Choose a reason for hiding this comment

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

There are 26 duplicated queries during this test?

Copy link
Member

Choose a reason for hiding this comment

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

Why is this configuration needed in this test?


return Task.CompletedTask;
});
}

// END OF TRAINING SECTION: Basic Orchard features tests.
61 changes: 61 additions & 0 deletions Lombiq.Tests.UI.Samples/Tests/DuplicatedSqlQueryDetectorTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using Lombiq.Tests.UI.Attributes;
using Lombiq.Tests.UI.Extensions;
using Lombiq.Tests.UI.Services;
using Lombiq.Tests.UI.Services.Counters.Configuration;
using Shouldly;
using System;
using System.Threading.Tasks;
using Xunit;
using Xunit.Abstractions;

namespace Lombiq.Tests.UI.Samples.Tests;

// Some times you may want to detect duplicated SQL queries. This can be useful if you want to make sure that your code
// does not execute the same query multiple times, wasting time and computing resources.
public class DuplicatedSqlQueryDetectorTests : UITestBase
{
public DuplicatedSqlQueryDetectorTests(ITestOutputHelper testOutputHelper)
: base(testOutputHelper)
{
}

// This test will fail because the app will read the same command result more times than the configured threshold
// during the Admin page rendering.
[Theory, Chrome]
public Task PageWithTooManyDuplicatedSqlQueriesShouldThrow(Browser browser) =>
Should.ThrowAsync<AggregateException>(() =>
ExecuteTestAfterSetupAsync(
context => context.SignInDirectlyAndGoToDashboardAsync(),
browser,
ConfigureAsync));

// This test will pass because not the Admin page was loaded.
[Theory, Chrome]
public Task PageWithoutDuplicatedSqlQueriesShouldPass(Browser browser) =>
Should.NotThrowAsync(() =>
Copy link
Member

Choose a reason for hiding this comment

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

Should.NotThrowAsync() is not needed.

ExecuteTestAfterSetupAsync(
async context => await context.GoToHomePageAsync(onlyIfNotAlreadyThere: false),
browser,
ConfigureAsync));

// We configure the test to throw an exception if a certain counter threshold is exceeded, but only in case of Admin
// pages.
private static Task ConfigureAsync(OrchardCoreUITestExecutorConfiguration configuration)
{
// The test is guaranteed to fail so we don't want to retry it needlessly.
configuration.MaxRetryCount = 0;

var adminCounterConfiguration = new CounterConfiguration();
// Let's enable and configure the counter threshold for ORM sessions.
adminCounterConfiguration.SessionThreshold.Disable = false;
adminCounterConfiguration.SessionThreshold.DbReaderReadThreshold = 0;
// Apply the configuration to the Admin pages only.
configuration.CounterConfiguration.Running.Add(
new RelativeUrlConfigurationKey(new Uri("/Admin", UriKind.Relative), exactMatch: false),
adminCounterConfiguration);

return Task.CompletedTask;
}
}

// END OF TRAINING SECTION: Duplicated SQL query detector.
1 change: 1 addition & 0 deletions Lombiq.Tests.UI.Samples/Tests/SecurityScanningTests.cs
Original file line number Diff line number Diff line change
@@ -161,3 +161,4 @@ protected override Task ExecuteTestAfterSetupAsync(
}

// END OF TRAINING SECTION: Security scanning.
// NEXT STATION: Head over to DuplicatedSqlQueryDetectorTests.cs.
65 changes: 65 additions & 0 deletions Lombiq.Tests.UI/Exceptions/CounterThresholdException.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
using Lombiq.Tests.UI.Services.Counters;
using System;
using System.Collections.Generic;
using System.Text;

namespace Lombiq.Tests.UI.Exceptions;

public class CounterThresholdException : Exception
{
public CounterThresholdException()
{
}

public CounterThresholdException(string message)
: this(probe: null, counter: null, value: null, message)
{
}

public CounterThresholdException(string message, Exception innerException)
: this(probe: null, counter: null, value: null, message, innerException)
{
}

public CounterThresholdException(
ICounterProbe probe,
ICounterKey counter,
ICounterValue value)
: this(probe, counter, value, message: null, innerException: null)
{
}

public CounterThresholdException(
ICounterProbe probe,
ICounterKey counter,
ICounterValue value,
string message)
: this(probe, counter, value, message, innerException: null)
{
}

public CounterThresholdException(
ICounterProbe probe,
ICounterKey counter,
ICounterValue value,
string message,
Exception innerException)
: base(FormatMessage(probe, counter, value, message), innerException)
{
}

private static string FormatMessage(
Copy link
Member

Choose a reason for hiding this comment

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

Generate exception messages that are easier to understand than:

DbExecuteCounterKey
SELECT [Document].* FROM [Document] WHERE [Document].[Type] = @type LIMIT 1
[0]Type = OrchardCore.Settings.SiteSettings, OrchardCore.Settings

IntegerCounterValue value: 26
Counter value is greater then DbCommandExecutionRepetitionThreshold, threshold: 22

I as an ordinary developer who didn't set up these counter thresholds but just wanted to change something unrelated but accidentally implemented a SELECT N+1 issue, should be able to understand what I did wrong.

Copy link
Member

Choose a reason for hiding this comment

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

While the current message is better, it's still cryptic when you first see it.

SessionProbe, [GET]https://localhost:9341/Admin
Database reader read counter
Query: SELECT [Document].* FROM [Document] WHERE [Document].[Type] = @type LIMIT 1
Parameters: [0]Type = OrchardCore.AdminMenu.Models.AdminMenuList, OrchardCore.AdminMenu
Count: 2
Counter value is greater then SessionThreshold.DbReaderReadThreshold, threshold: 0.

Start the exception with a message that explains, in plain English, what happened.

ICounterProbe probe,
ICounterKey counter,
ICounterValue value,
string message)
{
var builder = new StringBuilder();
if (probe is not null) builder.AppendLine(probe.DumpHeadline());
counter?.Dump().ForEach(line => builder.AppendLine(line));
value?.Dump().ForEach(line => builder.AppendLine(line));
if (!string.IsNullOrEmpty(message)) builder.AppendLine(message);

return builder.ToString();
}
}
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@
using Lombiq.Tests.UI.Helpers;
using Lombiq.Tests.UI.Pages;
using Lombiq.Tests.UI.Services;
using Lombiq.Tests.UI.Services.Counters;
using Newtonsoft.Json;
using OpenQA.Selenium;
using OpenQA.Selenium.Support.UI;
@@ -45,7 +46,10 @@ public static Task GoToAbsoluteUrlAsync(this UITestContext context, Uri absolute
await context.Configuration.Events.BeforeNavigation
.InvokeAsync<NavigationEventHandler>(eventHandler => eventHandler(context, absoluteUri));

context.Driver.Navigate().GoToUrl(absoluteUri);
using (new NavigationProbe(context.CounterDataCollector, absoluteUri))
{
context.Driver.Navigate().GoToUrl(absoluteUri);
}
Piedone marked this conversation as resolved.
Show resolved Hide resolved

await context.Configuration.Events.AfterNavigation
.InvokeAsync<NavigationEventHandler>(eventHandler => eventHandler(context, absoluteUri));
74 changes: 74 additions & 0 deletions Lombiq.Tests.UI/Services/CounterDataCollector.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
using Lombiq.Tests.UI.Services.Counters;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using Xunit.Abstractions;

namespace Lombiq.Tests.UI.Services;

public sealed class CounterDataCollector : CounterProbeBase, ICounterDataCollector
{
private readonly ITestOutputHelper _testOutputHelper;
private readonly ConcurrentBag<ICounterProbe> _probes = new();
private readonly ConcurrentBag<Exception> _postponedCounterExceptions = new();
public override bool IsAttached => true;
public Action<ICounterDataCollector, ICounterProbe> AssertCounterData { get; set; }
public string Phase { get; set; }

public CounterDataCollector(ITestOutputHelper testOutputHelper) =>
_testOutputHelper = testOutputHelper;

public void AttachProbe(ICounterProbe probe)
{
probe.CaptureCompleted = ProbeCaptureCompleted;
_probes.Add(probe);
}

public void Reset()
{
_probes.Clear();
_postponedCounterExceptions.Clear();
Clear();
}

public override void Increment(ICounterKey counter)
{
_probes.Where(probe => probe.IsAttached)
.ForEach(probe => probe.Increment(counter));
base.Increment(counter);
}

public override string DumpHeadline() => $"{nameof(CounterDataCollector)}, Phase = {Phase}";

public override IEnumerable<string> Dump()
{
var lines = new List<string>
{
DumpHeadline(),
};

lines.AddRange(DumpSummary().Select(line => $"\t{line}"));

return lines;
}

public void AssertCounter(ICounterProbe probe) => AssertCounterData?.Invoke(this, probe);

public void AssertCounter()
{
if (_postponedCounterExceptions.Any())
{
throw new AggregateException(
"There were exceptions out of the test execution context.",
_postponedCounterExceptions);
}

AssertCounter(this);
}

public void PostponeCounterException(Exception exception) => _postponedCounterExceptions.Add(exception);

private void ProbeCaptureCompleted(ICounterProbe probe) =>
probe.Dump().ForEach(_testOutputHelper.WriteLine);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
using Lombiq.Tests.UI.Services.Counters.Data;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Lombiq.Tests.UI.Services.Counters.Configuration;

public class CounterConfiguration
{
private const string WorkflowTypeStartActivitiesQuery =
"SELECT DISTINCT [Document].* FROM [Document] INNER JOIN [WorkflowTypeStartActivitiesIndex]"
+ " AS [WorkflowTypeStartActivitiesIndex_a1]"
+ " ON [WorkflowTypeStartActivitiesIndex_a1].[DocumentId] = [Document].[Id]"
+ " WHERE (([WorkflowTypeStartActivitiesIndex_a1].[StartActivityName] = @p0)"
+ " and ([WorkflowTypeStartActivitiesIndex_a1].[IsEnabled] = @p1))";

/// <summary>
/// Gets or sets the counter assertion method.
/// </summary>
public Action<ICounterDataCollector, ICounterProbe> AssertCounterData { get; set; }

/// <summary>
/// Gets or sets the exclude filter. Can be used to exclude counted values before assertion.
/// </summary>
public Func<ICounterKey, bool> ExcludeFilter { get; set; } = DefaultExcludeFilter;

/// <summary>
/// Gets or sets threshold configuration used under navigation requests. See:
/// <see cref="UI.Extensions.NavigationUITestContextExtensions.GoToAbsoluteUrlAsync(UITestContext, Uri, bool)"/>.
/// See: <see cref="NavigationProbe"/>.
/// </summary>
public CounterThresholdConfiguration NavigationThreshold { get; set; } = new CounterThresholdConfiguration
{
DbCommandIncludingParametersExecutionCountThreshold = 11,
DbCommandExcludingParametersExecutionThreshold = 22,
DbReaderReadThreshold = 11,
};

/// <summary>
/// Gets or sets threshold configuration used per <see cref="YesSql.ISession"/> lifetime. See:
/// <see cref="SessionProbe"/>.
/// </summary>
public CounterThresholdConfiguration SessionThreshold { get; set; } = new CounterThresholdConfiguration
{
DbCommandIncludingParametersExecutionCountThreshold = 22,
DbCommandExcludingParametersExecutionThreshold = 44,
DbReaderReadThreshold = 11,
};

/// <summary>
/// Gets or sets threshold configuration used per page load. See: <see cref="PageLoadProbe"/>.
/// </summary>
public CounterThresholdConfiguration PageLoadThreshold { get; set; } = new CounterThresholdConfiguration
{
DbCommandIncludingParametersExecutionCountThreshold = 22,
DbCommandExcludingParametersExecutionThreshold = 44,
DbReaderReadThreshold = 11,
};
Copy link
Member

Choose a reason for hiding this comment

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

Please add a piece of docs contrasting these three configs: when would you want to use each?


public static IEnumerable<ICounterKey> DefaultExcludeList { get; } = new List<ICounterKey>
{
new DbCommandExecuteCounterKey(
WorkflowTypeStartActivitiesQuery,
new List<KeyValuePair<string, object>>
{
new("p0", "ContentCreatedEvent"),
new("p1", value: true),
}),
new DbCommandExecuteCounterKey(
WorkflowTypeStartActivitiesQuery,
new List<KeyValuePair<string, object>>
{
new("p0", "ContentPublishedEvent"),
new("p1", value: true),
}),
new DbCommandExecuteCounterKey(
WorkflowTypeStartActivitiesQuery,
new List<KeyValuePair<string, object>>
{
new("p0", "ContentUpdatedEvent"),
new("p1", value: true),
}),
Copy link
Member

Choose a reason for hiding this comment

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

Why are these excluded?

};

public static bool DefaultExcludeFilter(ICounterKey key) => DefaultExcludeList.Contains(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider optimizing the DefaultExcludeFilter method if the default exclude list grows large. Using a more efficient lookup method, such as a hash set, could improve performance.

- public static bool DefaultExcludeFilter(ICounterKey key) => DefaultExcludeList.Contains(key);
+ public static bool DefaultExcludeFilter(ICounterKey key) => DefaultExcludeListHashSet.Contains(key);
+ private static HashSet<ICounterKey> DefaultExcludeListHashSet = new HashSet<ICounterKey>(DefaultExcludeList);

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
public static bool DefaultExcludeFilter(ICounterKey key) => DefaultExcludeList.Contains(key);
public static bool DefaultExcludeFilter(ICounterKey key) => DefaultExcludeListHashSet.Contains(key);
private static HashSet<ICounterKey> DefaultExcludeListHashSet = new HashSet<ICounterKey>(DefaultExcludeList);

}
Loading