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

Conversation

dministro
Copy link
Member

@dministro dministro commented Oct 24, 2022

OSOE-353
Fixes #93

Summary by CodeRabbit

  • New Features

    • Introduced a "Duplicated SQL query detector" to improve test efficiency and resource usage.
    • Added configuration settings for UI Testing Toolbox to enhance SQL query monitoring.
    • Implemented a new set of tests for detecting duplicated SQL queries.
  • Enhancements

    • Improved test environment with additional configuration options for database command execution thresholds.
    • Enhanced navigation and page load testing with new counter data collection and assertion capabilities.
    • Expanded testing toolbox with classes and interfaces to support performance metric collection.
  • Documentation

    • Updated the Readme to reflect new testing features and configurations.
  • Bug Fixes

    • Fixed issues related to database command execution tracking during tests.
  • Refactor

    • Refactored database connection and command wrappers to support performance data collection.
  • Tests

    • Added tests for new functionality and performance monitoring features.
  • Chores

    • Updated internal test infrastructure to incorporate new counter data collector services.

@github-actions
Copy link

This pull request has merge conflicts. Please resolve those before requesting a review.

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Impressive!

Lombiq.Tests.UI/Services/PhaseCounterConfiguration.cs Outdated Show resolved Hide resolved

namespace Lombiq.Tests.UI.Services;

public class PhaseCounterConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Mention this feature in the root Readme.

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be done.

/// <summary>
/// Gets the currently running <see cref="CounterDataCollector"/> instance.
/// </summary>
public CounterDataCollector CounterDataCollector { get; init; }
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be possible to be completely disabled too.

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be done. The page change dumps should also only be written when duplicate SQL query detection is enabled.

Lombiq.Tests.UI/Services/CounterDataCollector.cs Outdated Show resolved Hide resolved
Lombiq.Tests.UI/Services/Counters/CounterProbeBase.cs Outdated Show resolved Hide resolved
{
}

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.

Lombiq.Tests.UI/Services/CounterConfiguration.cs Outdated Show resolved Hide resolved
dministro and others added 16 commits November 7, 2022 17:31
Addressing "This needs to be possible to be completely disabled too."
Addressing ""human-readable" everywhere."
Addressing counting on page load not just navigation request
Addressing counting command text with and without parameters
Addressing "Keep the order of private fields and ctor assignments the same as the ctor parameters."
Addressing "Comment why this is necessary."
Addressing "After a page change, write all counters' values to the TestOutput for debugging."
Co-authored-by: Zoltán Lehóczky <[email protected]>
@Piedone
Copy link
Member

Piedone commented Dec 29, 2023

@coderabbitai review

I'm testing the AI code review tool CodeRabbit here. Please bear with me. And if it comments some useful feedback, please address it.

Copy link
Contributor

coderabbitai bot commented Dec 29, 2023

Walkthrough

Walkthrough

The changes implement a detection system for duplicated SQL queries and excessive row fetching during page loads, aiming to enhance testing efficiency and application performance. The system allows setting thresholds for query duplication and integrates with the UI Testing Toolbox. It introduces new classes and extensions for data collection, counter management, and performance probing, as well as modifications to existing services to support the new functionality.

Changes

File Path Change Summary
Lombiq.Tests.UI.Samples/Readme.md
Lombiq.Tests.UI.Samples/Tests/...
Added "Duplicated SQL query detector" and additional configuration settings for the UI Testing Toolbox.
Lombiq.Tests.UI/Exceptions/CounterThresholdException.cs
Lombiq.Tests.UI/Services/Counters/...
Lombiq.Tests.UI/Services/OrchardCoreHosting/OrchardApplicationFactory.cs
Lombiq.Tests.UI/Services/OrchardCoreInstance.cs
Lombiq.Tests.UI/Services/OrchardCoreUITestExecutorConfiguration.cs
Lombiq.Tests.UI/Services/UITestContext.cs
Lombiq.Tests.UI/Services/UITestExecutionSession.cs
Lombiq.Tests.UI/Services/UITestExecutor.cs
Introduced classes and changes related to counter threshold management, data collection, and performance probing.
Lombiq.Tests.UI/Services/ProbedConnectionFactory.cs
Lombiq.Tests.UI/Services/Counters/Data/ProbedDbConnection.cs
Lombiq.Tests.UI/Services/Counters/Data/ProbedDbCommand.cs
Lombiq.Tests.UI/Services/Counters/Data/ProbedDbDataReader.cs
Lombiq.Tests.UI/Services/Counters/Data/ProbedSqliteConnection.cs
Added classes for performance counting for database commands and connections.

Assessment against linked issues

Objective Addressed Explanation
Implement a feature to detect duplicated SQL queries during page load (#93)
Allow configuration of a threshold for query duplication (#93)
Use Orchard Core Mini Profiler for query detection or implement similar logic (#93) The summary does not explicitly mention the Mini Profiler, but similar profiling logic may have been implemented.
Detect queries fetching too many rows and implement configurable thresholds (#93) The summary mentions detection and thresholds, but it's unclear if this specifically includes row fetching.
Leverage common APIs for integration with Jira issue OSOE-353 (#93) There is no specific mention of leveraging common APIs, but the extensive changes suggest some level of integration may have been achieved.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

coderabbitai bot commented Dec 29, 2023

@Piedone, I'm ready to review the pull request as requested. Please provide any specific files or code snippets you would like me to focus on, or I will proceed with a general review of the changes.

Action performed

Review triggered.

@Piedone
Copy link
Member

Piedone commented Dec 29, 2023

@coderabbitai review

Sorry about that.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a1a4cde and 8c58730.
Files selected for processing (47)
  • Lombiq.Tests.UI.Samples/Readme.md (1 hunks)
  • Lombiq.Tests.UI.Samples/Tests/BasicOrchardFeaturesTests.cs (1 hunks)
  • Lombiq.Tests.UI.Samples/Tests/DuplicatedSqlQueryDetectorTests.cs (1 hunks)
  • Lombiq.Tests.UI.Samples/Tests/InteractiveModeTests.cs (1 hunks)
  • Lombiq.Tests.UI/Exceptions/CounterThresholdException.cs (1 hunks)
  • Lombiq.Tests.UI/Extensions/NavigationUITestContextExtensions.cs (2 hunks)
  • Lombiq.Tests.UI/Services/CounterDataCollector.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Configuration/CounterConfiguration.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Configuration/CounterConfigurations.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Configuration/CounterThresholdConfiguration.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Configuration/PhaseCounterConfiguration.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Configuration/RelativeUrlConfigurationKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Configuration/RunningPhaseCounterConfiguration.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/CounterKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/CounterProbe.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/CounterProbeBase.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/DbCommandCounterKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/DbCommandExecuteCounterKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/DbCommandTextExecuteCounterKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/DbReaderReadCounterKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/ProbedDbCommand.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/ProbedDbConnection.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/ProbedDbDataReader.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Data/ProbedSqliteConnection.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Extensions/CounterDataCollectorExtensions.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Extensions/RelativeUrlConfigurationKeyExtensions.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Extensions/RunningPhaseCounterConfigurationExtensions.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/ICounterConfigurationKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/ICounterDataCollector.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/ICounterKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/ICounterProbe.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/ICounterValue.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/IOutOfTestContextCounterProbe.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/IRelativeUrlConfigurationKey.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Middlewares/PageLoadProbeMiddleware.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/NavigationProbe.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/PageLoadProbe.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/SessionProbe.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Value/CounterValue.cs (1 hunks)
  • Lombiq.Tests.UI/Services/Counters/Value/IntegerCounterValue.cs (1 hunks)
  • Lombiq.Tests.UI/Services/OrchardCoreHosting/OrchardApplicationFactory.cs (5 hunks)
  • Lombiq.Tests.UI/Services/OrchardCoreInstance.cs (3 hunks)
  • Lombiq.Tests.UI/Services/OrchardCoreUITestExecutorConfiguration.cs (2 hunks)
  • Lombiq.Tests.UI/Services/ProbedConnectionFactory.cs (1 hunks)
  • Lombiq.Tests.UI/Services/UITestContext.cs (2 hunks)
  • Lombiq.Tests.UI/Services/UITestExecutionSession.cs (7 hunks)
  • Lombiq.Tests.UI/Services/UITestExecutor.cs (5 hunks)
Files skipped from review due to trivial changes (1)
  • Lombiq.Tests.UI.Samples/Tests/InteractiveModeTests.cs
Additional comments: 69
Lombiq.Tests.UI/Services/Counters/ICounterConfigurationKey.cs (1)
  • 1-10: The interface ICounterConfigurationKey is well-defined as a marker interface for type safety and comparison in configurations.
Lombiq.Tests.UI/Services/Counters/IOutOfTestContextCounterProbe.cs (1)
  • 1-9: The interface IOutOfTestContextCounterProbe correctly extends ICounterProbe to specialize for probes operating outside of the test context.
Lombiq.Tests.UI/Services/Counters/Value/IntegerCounterValue.cs (1)
  • 1-14: The IntegerCounterValue class provides a concrete implementation for integer counter values with an appropriate string representation.
Lombiq.Tests.UI/Services/Counters/Value/CounterValue.cs (1)
  • 1-15: The CounterValue<TValue> class is well-designed as a generic base for counter values, with a default display name and a mechanism to dump the value.
Lombiq.Tests.UI/Services/Counters/Configuration/PhaseCounterConfiguration.cs (1)
  • 1-14: The PhaseCounterConfiguration class is correctly set up with default threshold values for different application phases.
Lombiq.Tests.UI/Services/Counters/IRelativeUrlConfigurationKey.cs (1)
  • 1-19: The IRelativeUrlConfigurationKey interface extends ICounterConfigurationKey with additional properties for URL-based configuration keys.
Lombiq.Tests.UI/Services/Counters/ICounterValue.cs (1)
  • 1-20: The ICounterValue interface defines a clear contract for counter values with a DisplayName property and a Dump method.
Lombiq.Tests.UI/Services/Counters/ICounterKey.cs (1)
  • 1-21: The ICounterKey interface is well-defined, providing a clear contract for implementing counter keys with a DisplayName property and a Dump method.
Lombiq.Tests.UI/Services/Counters/Configuration/RelativeUrlConfigurationKey.cs (1)
  • 1-22: The RelativeUrlConfigurationKey class correctly implements IRelativeUrlConfigurationKey with proper equality comparison and hash code generation.
Lombiq.Tests.UI/Services/Counters/CounterKey.cs (1)
  • 1-16: The CounterKey abstract class provides a solid base for counter keys with necessary abstract members for concrete implementations.
Lombiq.Tests.UI/Services/Counters/Data/DbReaderReadCounterKey.cs (1)
  • 1-22: The DbReaderReadCounterKey class is well-defined, providing a specific counter key for database reader read operations.
Lombiq.Tests.UI/Services/Counters/Middlewares/PageLoadProbeMiddleware.cs (1)
  • 1-24: The PageLoadProbeMiddleware class is correctly defined, integrating the PageLoadProbe into the request pipeline.
Lombiq.Tests.UI/Services/Counters/Data/DbCommandExecuteCounterKey.cs (1)
  • 1-22: The DbCommandExecuteCounterKey class is well-defined, providing a specific counter key for database command executions with parameters.
Lombiq.Tests.UI/Services/Counters/Extensions/RelativeUrlConfigurationKeyExtensions.cs (1)
  • 1-20: The RelativeUrlConfigurationKeyExtensions class provides a clear and correct implementation for comparing relative URL configuration keys.
Lombiq.Tests.UI/Services/Counters/NavigationProbe.cs (1)
  • 1-24: The NavigationProbe class is correctly implemented, providing a concrete implementation for navigation probes with URL configuration.
Lombiq.Tests.UI/Services/Counters/Data/DbCommandTextExecuteCounterKey.cs (1)
  • 1-28: The DbCommandTextExecuteCounterKey class is well-defined, providing a specific counter key for database command executions based on command text.
Lombiq.Tests.UI/Services/Counters/PageLoadProbe.cs (1)
  • 1-28: The PageLoadProbe class is correctly implemented, providing a concrete implementation for page load probes with URL configuration.
Lombiq.Tests.UI/Services/ProbedConnectionFactory.cs (1)
  • 1-33: The ProbedConnectionFactory class is well-designed, providing a mechanism to create probed connections for performance tracking.
Lombiq.Tests.UI/Services/Counters/ICounterDataCollector.cs (1)
  • 1-36: The ICounterDataCollector interface provides a comprehensive contract for a counter data collector with methods for managing probes and asserting data.
Lombiq.Tests.UI/Services/Counters/Data/ProbedSqliteConnection.cs (1)
  • 1-36: The ProbedSqliteConnection class is correctly implemented, providing a probed connection specifically for Sqlite with the necessary overrides for performance tracking.
Lombiq.Tests.UI/Services/Counters/ICounterProbe.cs (1)
  • 1-45: The ICounterProbe interface defines a clear contract for implementing probes in the counter infrastructure with properties and methods for managing and dumping counter data.
Lombiq.Tests.UI/Services/Counters/Configuration/CounterThresholdConfiguration.cs (1)
  • 1-32: The CounterThresholdConfiguration class provides a clear configuration structure for setting thresholds in the counter infrastructure.
Lombiq.Tests.UI.Samples/Tests/BasicOrchardFeaturesTests.cs (1)
  • 25-38: The configuration lambda in BasicOrchardFeaturesShouldWork correctly sets the DbCommandExecutionThreshold. Ensure that the threshold value of 26 is based on known requirements or empirical data to avoid false positives or negatives in tests.
Lombiq.Tests.UI/Exceptions/CounterThresholdException.cs (1)
  • 8-64: The constructors of CounterThresholdException are well-structured, providing multiple overloads for different use cases. The FormatMessage method is also well-implemented, ensuring that all relevant information is included in the exception message.
Lombiq.Tests.UI/Services/Counters/Data/DbCommandCounterKey.cs (1)
  • 8-56: The DbCommandCounterKey class correctly encapsulates the command text and parameters, providing a robust Equals method for comparison. The Dump method is also well-implemented, providing detailed information for debugging purposes.
Lombiq.Tests.UI/Services/Counters/Configuration/RunningPhaseCounterConfiguration.cs (1)
  • 8-43: The RunningPhaseCounterConfiguration class correctly implements the IDictionary interface, providing thread-safe access to counter configurations. The use of ConcurrentDictionary is appropriate for concurrent access scenarios.
Lombiq.Tests.UI/Services/Counters/Extensions/RunningPhaseCounterConfigurationExtensions.cs (1)
  • 11-60: The extension methods in RunningPhaseCounterConfigurationExtensions are well-designed, providing a fluent interface for configuring counter thresholds. The use of GetMaybeByKey to avoid key not found exceptions is a good practice.
Lombiq.Tests.UI/Services/CounterDataCollector.cs (1)
  • 10-73: The CounterDataCollector class is well-implemented, with clear responsibilities and proper encapsulation of its members. The AssertCounter method correctly handles postponed exceptions, ensuring they are not lost.
Lombiq.Tests.UI.Samples/Readme.md (1)
  • 27-30: The addition of "Duplicated SQL query detector" to the Readme.md file is clear and follows the existing format of the document. This update should help users find information about the new feature.
Lombiq.Tests.UI/Services/Counters/CounterProbeBase.cs (1)
  • 9-77: The CounterProbeBase class provides a solid foundation for counter probes with a clear and concise implementation. The DumpSummary method is particularly well-done, providing a summary of the counters in a readable format.
Lombiq.Tests.UI.Samples/Tests/DuplicatedSqlQueryDetectorTests.cs (1)
  • 13-58: The test methods in DuplicatedSqlQueryDetectorTests are well-structured, and the configuration method ConfigureAsync is correctly setting up the counter thresholds. Ensure that the threshold values are based on the expected behavior of the application to avoid false positives.
Lombiq.Tests.UI/Services/Counters/Extensions/CounterDataCollectorExtensions.cs (1)
  • 9-64: The extension methods in CounterDataCollectorExtensions correctly increment the appropriate counters before executing the database commands. This ensures that all database interactions are tracked, which is essential for detecting duplicated SQL queries.
Lombiq.Tests.UI/Services/Counters/SessionProbe.cs (4)
  • 12-12: The SessionProbe class correctly implements ISession and IRelativeUrlConfigurationKey interfaces, which is consistent with the PR objectives to enhance performance monitoring.

  • 20-26: The constructor of SessionProbe properly initializes its properties and the underlying session object. It's good practice to validate the non-nullness of the session parameter since it's a critical dependency.

  • 55-61: The asynchronous disposal pattern in DisposeAsync is correctly implemented, ensuring that the session is disposed of and then the base disposal logic is called. This is important for proper resource management.

  • 63-68: The implementation of IRelativeUrlConfigurationKey in SessionProbe is consistent and provides the necessary properties and method for URL configuration. This aligns with the PR's goal of enhancing performance monitoring.

Lombiq.Tests.UI/Services/Counters/Data/ProbedDbConnection.cs (3)
  • 13-13: The ProbedDbConnection class is designed to wrap a DbConnection and collect performance data, which aligns with the PR's objectives to monitor database interactions.

  • 37-42: The constructor of ProbedDbConnection correctly initializes the connection and the counter data collector, including a null check and event subscription for state changes. This is crucial for the reliability of the performance tracking feature.

  • 80-90: The disposal pattern in ProbedDbConnection is correctly implemented, ensuring that the wrapped connection is disposed of and the event handler is unsubscribed, which is important to prevent memory leaks.

Lombiq.Tests.UI/Services/Counters/Configuration/CounterConfiguration.cs (2)
  • 8-8: The CounterConfiguration class provides a central place to manage various threshold configurations and exclude filters, which is essential for the new feature's configurability as described in the PR objectives.

  • 10-15: The SQL query constant within CounterConfiguration is used in the default exclude list, which suggests it's a known query that should not trigger performance warnings. This is a good practice to prevent false positives in performance monitoring.

Lombiq.Tests.UI/Services/Counters/Configuration/CounterConfigurations.cs (3)
  • 11-11: The CounterConfigurations class holds different counter configurations for various phases of the web application, which is in line with the PR's objectives to provide detailed performance monitoring.

  • 23-73: The DefaultAssertCounterData method in CounterConfigurations contains complex logic to assert counter data. Ensure that this logic is thoroughly tested to prevent any issues during runtime.

  • 75-95: The AssertIntegerCounterValue method correctly throws a CounterThresholdException when a counter value exceeds the threshold. This is a key part of the feature to detect performance issues.

Lombiq.Tests.UI/Services/Counters/Data/ProbedDbCommand.cs (3)
  • 13-13: The ProbedDbCommand class is designed to wrap a DbCommand and collect performance data, which aligns with the PR's objectives to monitor database command executions.

  • 71-81: The constructor of ProbedDbCommand correctly initializes the command and the counter data collector, including a null check. This is crucial for the reliability of the performance tracking feature.

  • 122-131: The disposal pattern in ProbedDbCommand is correctly implemented, ensuring that the wrapped command is disposed of, which is important to prevent resource leaks.

Lombiq.Tests.UI/Services/Counters/Data/ProbedDbDataReader.cs (4)
  • 12-12: The ProbedDbDataReader class is designed to wrap a DbDataReader and collect performance data, which aligns with the PR's objectives to monitor database interactions.

  • 39-55: The constructor of ProbedDbDataReader correctly initializes the reader, command, and counter data collector, including a null check and the initialization of the counter key. This is crucial for accurate performance tracking.

  • 119-133: The Read and ReadAsync methods in ProbedDbDataReader correctly increment the performance counter when a row is read, which is essential for tracking the number of rows fetched by a query.

  • 139-143: The disposal pattern in ProbedDbDataReader is correctly implemented, ensuring that the wrapped reader is disposed of, which is important to prevent resource leaks.

Lombiq.Tests.UI/Services/UITestExecutor.cs (1)
  • 15-18: The introduction of a SemaphoreSlim for limiting concurrent tests is a good practice for resource management. Ensure that all usages of _numberOfTestsLimit properly handle the semaphore's release, even in the case of exceptions.
Lombiq.Tests.UI/Services/OrchardCoreHosting/OrchardApplicationFactory.cs (2)
  • 30-34: The use of ConcurrentBag for _createdStores is appropriate for thread-safe operations. Ensure that all interactions with _createdStores are thread-safe and consider the implications of unordered access inherent to ConcurrentBag.

  • 152-172: The AddSessionProbe method correctly replaces the ISession service with SessionProbe. Verify that the SessionProbe implementation is compatible with all expected usages of ISession throughout the application.

Lombiq.Tests.UI/Services/OrchardCoreUITestExecutorConfiguration.cs (1)
  • 175-178: The addition of the CounterConfiguration property aligns with the PR's objective to introduce performance monitoring. Ensure that the CounterConfigurations type is correctly defined and used throughout the application.
Lombiq.Tests.UI/Services/OrchardCoreInstance.cs (1)
  • 60-75: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [54-72]

The integration of CounterDataCollector into OrchardCoreInstance is consistent with the PR's objectives. Ensure that the CounterDataCollector is properly utilized within the OrchardCoreInstance and that its lifecycle is managed correctly.

Lombiq.Tests.UI/Services/UITestContext.cs (2)
  • 102-105: The addition of the CounterDataCollector property to UITestContext introduces a new dependency that will be used for performance tracking. Ensure that all necessary null checks and exception handling are in place where this new property is utilized.

  • 99-117: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [113-124]

The constructor now initializes the CounterDataCollector property. Ensure that all instantiations of UITestContext throughout the codebase are updated to provide this new parameter.

Verification successful

The output from the shell script indicates that there is an instantiation of UITestContext in the file UITestExecutionSession.cs that includes the new CounterDataCollector parameter. This suggests that at least one place in the codebase has been updated to accommodate the new constructor signature.

However, this does not guarantee that all instances have been updated accordingly. To ensure that all instantiations of UITestContext have been updated, we would need to check for any other occurrences that do not include the new parameter.

Given the information at hand, the review comment is partially verified, as we have found at least one instance where the UITestContext is correctly instantiated with the new parameter. To fully verify the comment, we would need to check for all instances of UITestContext instantiation.


The output from the shell script indicates that the only instantiation of UITestContext found in the codebase is the one in UITestExecutionSession.cs, which correctly includes the new CounterDataCollector parameter. This suggests that the necessary update to the UITestContext constructor has been made at least in this instance.

Since no other instantiations were found by the script, it appears that either there are no other instantiations in the codebase or they have already been updated to include the new parameter. Therefore, based on the information provided by the script, the review comment concerning the update of UITestContext instantiations has been verified.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for instantiations of UITestContext without the new parameter.
ast-grep --lang csharp --pattern $'new UITestContext($_, $_, $_, $_, $_, $_, $_)'

Length of output: 840



Script:

#!/bin/bash
# Search for all instantiations of UITestContext to ensure they include the new parameter.
ast-grep --lang csharp --pattern $'new UITestContext($$$)'

Length of output: 817

Lombiq.Tests.UI/Extensions/NavigationUITestContextExtensions.cs (2)
  • 6-6: The import statement for Lombiq.Tests.UI.Services.Counters has been added to support the new NavigationProbe functionality. Verify that this namespace is used elsewhere in the file to avoid unnecessary imports.
Verification successful

The search for the usage of the namespace Lombiq.Tests.UI.Services.Counters in the file Lombiq.Tests.UI/Extensions/NavigationUITestContextExtensions.cs only returned the import statement itself. This suggests that the namespace is not used elsewhere in the file, which could mean that the import is unnecessary. However, since the AI-generated summary mentioned that a NavigationProbe object is being used within a using block, it's possible that the NavigationProbe class is part of this namespace and the search result is incomplete due to the scope of the search command.

To verify the actual usage of the NavigationProbe within the file, a more detailed search is needed to check for instances of NavigationProbe being instantiated or used.


The search confirmed that the NavigationProbe class is indeed used within the Lombiq.Tests.UI/Extensions/NavigationUITestContextExtensions.cs file. This validates the necessity of the import statement for the Lombiq.Tests.UI.Services.Counters namespace, as it contains the NavigationProbe class which is utilized in a using block.

Based on this information, the import statement is correctly placed and is required for the new functionality introduced in the pull request.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for usage of Lombiq.Tests.UI.Services.Counters in the file.
rg --type cs 'Lombiq.Tests.UI.Services.Counters' Lombiq.Tests.UI/Extensions/NavigationUITestContextExtensions.cs

Length of output: 153



Script:

#!/bin/bash
# Search for instances of NavigationProbe being used in the file.
rg --type cs 'NavigationProbe' Lombiq.Tests.UI/Extensions/NavigationUITestContextExtensions.cs

Length of output: 181

* 49-52: The `context.Driver.Navigate().GoToUrl(absoluteUri)` call is now wrapped in a `using` block with a `NavigationProbe`. This is a good use of the `using` statement for resource management, ensuring that the probe is disposed of correctly after navigation. Confirm that the `NavigationProbe` correctly implements `IDisposable`.
Lombiq.Tests.UI/Services/UITestExecutionSession.cs (9)
  • 9-9: The addition of the CounterDataCollector configuration namespace is noted and seems appropriate for the new functionality being introduced.

  • 109-109: Resetting the CounterDataCollector at the beginning of the test execution ensures that the counters start from a clean state for each test run.

  • 110-112: Setting the phase to Running and asserting counter data based on the running configuration is a good practice to ensure that the test is aware of the current phase and can validate the counters accordingly.

  • 120-121: Dumping and asserting the counter data at the end of the test execution is crucial for identifying any duplicated SQL queries or excessive row fetching. This should help in catching performance issues early.

  • 483-486: Configuring the CounterDataCollector during the setup phase with the appropriate phase and assertion data is consistent with the changes in the previous hunk and aligns with the objectives of the PR.

  • 495-496: The operations to dump and assert counter data after the setup operation are consistent with the intent to monitor performance issues during all phases of test execution.

  • 515-515: Catching CounterThresholdException explicitly allows the test to fail fast if performance thresholds are exceeded during setup, which is a critical part of performance testing.

  • 619-625: Instantiating a new CounterDataCollector and passing it to the OrchardCoreInstance constructor ensures that counter data is collected throughout the application lifecycle.

  • 660-661: Passing the CounterDataCollector to the UITestContext constructor ensures that the test context has access to the counter data, which is necessary for the new performance monitoring features.

Comment on lines +173 to +183
private static async Task PrepareTestLimitAsync(OrchardCoreUITestExecutorConfiguration configuration)
{
await _numberOfTestsLimitLock.WaitAsync();

if (_numberOfTestsLimit == null && configuration.MaxParallelTests > 0)
{
_numberOfTestsLimit = new SemaphoreSlim(configuration.MaxParallelTests);
}

_numberOfTestsLimitLock.Release();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The PrepareTestLimitAsync method correctly initializes the _numberOfTestsLimit semaphore within a lock to ensure thread safety. However, the lock release should be in a finally block to guarantee it's released even if an exception occurs.

- _numberOfTestsLimitLock.Release();
+ finally
+ {
+     _numberOfTestsLimitLock.Release();
+ }

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
private static async Task PrepareTestLimitAsync(OrchardCoreUITestExecutorConfiguration configuration)
{
await _numberOfTestsLimitLock.WaitAsync();
if (_numberOfTestsLimit == null && configuration.MaxParallelTests > 0)
{
_numberOfTestsLimit = new SemaphoreSlim(configuration.MaxParallelTests);
}
_numberOfTestsLimitLock.Release();
}
private static async Task PrepareTestLimitAsync(OrchardCoreUITestExecutorConfiguration configuration)
{
await _numberOfTestsLimitLock.WaitAsync();
try
{
if (_numberOfTestsLimit == null && configuration.MaxParallelTests > 0)
{
_numberOfTestsLimit = new SemaphoreSlim(configuration.MaxParallelTests);
}
}
finally
{
_numberOfTestsLimitLock.Release();
}
}

Comment on lines +7 to +71
public abstract class CounterProbe : CounterProbeBase, IDisposable
{
private bool _disposed;

public override bool IsAttached => !_disposed;
public ICounterDataCollector CounterDataCollector { get; init; }

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

lines.AddRange(
Counters.SelectMany(entry =>
entry.Key.Dump()
.Concat(entry.Value.Dump().Select(line => $"\t{line}")))
.Concat(DumpSummary())
.Select(line => $"\t{line}"));

return lines;
}

protected CounterProbe(ICounterDataCollector counterDataCollector)
{
CounterDataCollector = counterDataCollector;
CounterDataCollector.AttachProbe(this);
}

protected virtual void OnAssertData() =>
CounterDataCollector.AssertCounter(this);

protected virtual void OnDisposing()
{
}

protected virtual void OnDisposed()
{
}

protected virtual void Dispose(bool disposing)
{
if (!_disposed)
{
OnDisposing();
try { OnAssertData(); }
finally
{
if (disposing)
{
OnDisposed();
}

_disposed = true;
OnCaptureCompleted();
}
}
}

public void Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of IDisposable in CounterProbe is correct, following the standard dispose pattern. However, the empty virtual methods OnDisposing and OnDisposed could be removed if they are not overridden in derived classes to reduce unnecessary code.

Copy link
Contributor

coderabbitai bot commented Dec 29, 2023

@Piedone, no worries at all! I'm here to assist you. If you have any specific areas or files in the pull request that you would like me to review, please let me know. Otherwise, I will proceed with a general review of the changes.

Action performed

Review triggered.

@Piedone
Copy link
Member

Piedone commented Dec 29, 2023

These comments seem useful, please check them out. You can also chat with the bot, see the tips under #216 (comment).

Comment on lines 5 to 8
/// <summary>
/// Gets or sets a value indicating whether the current threshold configuration and checking is disabled.
/// </summary>
public bool Disable { get; set; } = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The property Disable should be renamed to IsEnabled and its logic inverted for consistency with common API naming conventions. This change also addresses a previous review comment.

- public bool Disable { get; set; } = true;
+ public bool IsEnabled { get; set; } = false;

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
/// <summary>
/// Gets or sets a value indicating whether the current threshold configuration and checking is disabled.
/// </summary>
public bool Disable { get; set; } = true;
/// <summary>
/// Gets or sets a value indicating whether the current threshold configuration and checking is disabled.
/// </summary>
public bool IsEnabled { get; set; } = false;

}),
};

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);

Comment on lines 32 to 58
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add duplicate SQL query detector (OSOE-353)
4 participants