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

Add tests #5

Merged
merged 31 commits into from
Oct 21, 2022
Merged

Add tests #5

merged 31 commits into from
Oct 21, 2022

Conversation

kentico-ericd
Copy link
Member

Motivation

Adds unit tests to the integration

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

tests/Kentico.Xperience.Algolia.Tests.csproj Outdated Show resolved Hide resolved
tests/Kentico.Xperience.Algolia.Tests.csproj Outdated Show resolved Hide resolved
tests/Directory.Build.props Show resolved Hide resolved
tests/Base/AlgoliaTest.cs Outdated Show resolved Hide resolved
tests/Base/AlgoliaTest.cs Outdated Show resolved Hide resolved
tests/Tests/IAlgoliaIndexServiceTests.cs Outdated Show resolved Hide resolved

namespace Kentico.Xperience.Algolia.Test
{
internal class IAlgoliaClientTests
Copy link
Contributor

Choose a reason for hiding this comment

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

Test is testing class not interface, rename the class.
Add tests for GetStatistics(...) method as well, check that cache is used and underlying search client is used.
Add tests for Rebuild(...) method as well, check that underlying search index method is called with expected parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added GetStatistics test in 7d647c6, please let me know if it's correct.

For Rebuild, because it uses DocumentQuery I don't believe I can test that unless we use isolated integration tests, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I don't know whether is possible to disconnect MultiDocumentQuery from database. Someone from CM1 team will give you a hint. If it's not possible introduce isolated integration test.

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 added tests in 887d7be by replacing MultiDocumentQuery with IPageRetriever, let me know if it looks okay

tests/Tests/IAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Tests/IAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Tests/IAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Kentico.Xperience.Algolia.Tests.csproj Outdated Show resolved Hide resolved
tests/Kentico.Xperience.Algolia.Tests.csproj Show resolved Hide resolved
tests/Tests/IAlgoliaObjectGeneratorTests.cs Outdated Show resolved Hide resolved
tests/Tests/IAlgoliaObjectGeneratorTests.cs Outdated Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Show resolved Hide resolved
tests/Article.generated.cs Outdated Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Outdated Show resolved Hide resolved
tests/Tests/DefaultAlgoliaClientTests.cs Show resolved Hide resolved
await algoliaClient.Rebuild(nameof(ArticleEnSearchModel), CancellationToken.None);

await mockPageRetriever.Received(1).RetrieveMultipleAsync(Arg.Any<Action<MultiDocumentQuery>>());
await mockIndexService.Received(1).InitializeIndex(nameof(ArticleEnSearchModel), Arg.Any<CancellationToken>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Check that cancellation token is passed to inner call. Amend this also in another tests, namely:

  • GetStatistics_CallsMethods
  • DeleteRecords_ValidIndex_ReturnsProcessedCount
  • UpsertRecords_ValidIndex_ReturnsProcessedCount
  • CrawlUrls_HttpClientReceivesParameters
  • GetCrawler_CallsMethods
  • ProcessAlgoliaTasks_ValidTasks_ProcessesTasks
  • ProcessCrawlerTasks_ValidTasks_ReturnsProcessedCount

Copy link
Member Author

@kentico-ericd kentico-ericd Oct 19, 2022

Choose a reason for hiding this comment

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

@kentico-jaroslavn I added a missing CancellationToken usage in 8ca352a but I'm unsure what else the tests should check for. The tests are already checking Arg.Any<CancellationToken>() which ensures that the token is non-null

Copy link
Contributor

Choose a reason for hiding this comment

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

Test should check that the same instance of cancellation token is passed to underlying method.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kentico-jaroslavn I added CancellationToken tests in 7db1794. But, there are 2 tests that I had an issue with: GetCrawler_CallsMethods and CrawlUrls_HttpClientReceivesParameters. It seems that when MockHttpMessageHandler.SendAsync() is called, it receives a different token, probably set from the HttpClient. I cannot find a way to ensure that it receives the same token that is passed through other methods, do you have any advice?

await algoliaClient.Rebuild(nameof(ArticleEnSearchModel), CancellationToken.None);

await mockPageRetriever.Received(1).RetrieveMultipleAsync(Arg.Any<Action<MultiDocumentQuery>>());
await mockIndexService.Received(1).InitializeIndex(nameof(ArticleEnSearchModel), Arg.Any<CancellationToken>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Test should check that the same instance of cancellation token is passed to underlying method.

@@ -32,10 +33,13 @@ public void HandleEvent(TreeNode node, string eventName)
var taskType = GetTaskType(node, eventName);

// Check crawlers
foreach (var crawlerId in IndexStore.Instance.GetAllCrawlers())
if (node.Site.SiteStatus == SiteStatusEnum.Running)
Copy link
Contributor

Choose a reason for hiding this comment

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

Indexing the structured content when site is stopped is intended, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I believe that is the desired behavior for structured content. If the site is stopped for emergency fixes or large page updates, when it is started again the index will already be up-to-date. For crawlers that's unfortunately not possible since the pages can't be crawled while offline.

@kentico-ericd kentico-ericd merged commit 721c6c4 into master Oct 21, 2022
@kentico-ericd kentico-ericd deleted the add-tests branch October 21, 2022 15:15
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.

None yet

2 participants