[PM-3626] Add Collections & Organizations Tests #68

lizard-boy · 2024-10-19T09:06:40Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Add tests to confirm the exact same behavior across databases.

Code changes

test/Infrastructure.EFIntegration.Test/Repositories/OrganizationRepositoryTests.cs: Delete tests that have been moved
test/Infrastructure.IntegrationTest/Repositories/CollectionRepositoryTests.cs: Totally new collection repository tests that verify that we are retrieving the same collection permissions across DB's.
test/Infrastructure.IntegrationTest/Repositories/OrganizationRepositoryTests.cs: New and migrated tests for various IOrganizationRepository things.
test/Infrastructure.IntegrationTest/restore.sh: Add helper script for instantiating all databases into a seperate database from what you normally use so that you can blow it up and start over a bit easier.
test/Infrastructure.IntegrationTest/ServiceProviderExtensions.cs: Various helpers that will help in writing tests quicker.

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

Greptile Summary

This pull request adds and updates tests for collections and organizations to ensure consistent behavior across different databases. Key changes include:

Added new integration tests in test/Infrastructure.IntegrationTest/Repositories/ for CollectionCipher, Collection, Device, Group, Installation, Organization, Policy, and TaxRate repositories
Removed redundant tests from test/Infrastructure.EFIntegration.Test/Repositories/ directory
Introduced ServiceProviderExtensions.cs with utility methods for creating test data
Added restore.sh script to set up test databases for different providers
Refactored and optimized database queries in CollectionCipherRepository.cs and GroupRepository.cs

- Add Helper Script for creating seperated databases for tests

- Add Tests - Prefer Method Syntax

greptile-apps

_{27 file(s) reviewed, 24 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-19T09:10:28Z

test/Infrastructure.EFIntegration.Test/Repositories/EqualityComparers/CollectionCompare.cs

logic: The GetHashCode method was not properly implemented. It used the base GetHashCode, which doesn't consider the specific properties of the Collection object. If this class is still needed elsewhere, consider reimplementing it with a proper GetHashCode method.

greptile-apps · 2024-10-19T09:11:02Z

test/Infrastructure.EFIntegration.Test/Repositories/EqualityComparers/InstallationCompare.cs

logic: The GetHashCode method was incorrectly implemented, returning the base GetHashCode instead of a hash based on the Installation properties. If this class is needed elsewhere, it should be reimplemented with a correct GetHashCode method.

greptile-apps · 2024-10-19T09:11:52Z

test/Infrastructure.EFIntegration.Test/Repositories/OrganizationRepositoryTests.cs

@@ -148,42 +81,4 @@ public async void GetManyAbilitiesAsync_Works(SqlRepo.OrganizationRepository sql
        list.Concat(await sqlOrganizationRepo.GetManyAbilitiesAsync());


logic: Same issue with list.Concat() here. Use list.AddRange()

greptile-apps · 2024-10-19T09:12:51Z

test/Infrastructure.IntegrationTest/Repositories/CollectionRepositoryTests.cs

+        // The user will be apart of two groups, one that gives them little access to the collection
+        // and one that gives them fully access, via our rules, they should be given the full rights.


syntax: Typo: 'apart' should be 'a part'

greptile-apps · 2024-10-19T09:13:24Z

test/Infrastructure.IntegrationTest/Repositories/GroupRepositoryTests.cs

+        var group2 = groupsAndCollections.FirstOrDefault(t => t.Item1.ExternalId == groupExternalIds[1]);
+        Assert.NotNull(group2);
+        Assert.Single(group2!.Item2);
+        var group2Collection = Assert.Single(group1!.Item2);


logic: group2Collection is using group1 instead of group2

greptile-apps · 2024-10-19T09:14:46Z

test/Infrastructure.IntegrationTest/ServiceProviderExtensions.cs

+    public static async Task<IReadOnlyList<T>> CreateManyAsync<TService, T>(this IServiceProvider services, Func<TService, int, Task<T>> creator, int number)
+        where TService : class
+    {
+        var service = services.GetRequiredService<TService>();
+
+        var items = new T[number];
+        for (var i = 0; i < number; i++)
+        {
+            items[i] = await creator(service, i);
+        }
+        return items;
+    }


style: Consider using a more efficient approach for creating multiple entities, such as bulk insert

greptile-apps · 2024-10-19T09:14:47Z

test/Infrastructure.IntegrationTest/ServiceProviderExtensions.cs

+    public static async Task<IReadOnlyList<T>> CreateManyAsync<TService, T>(this IServiceProvider services, Func<TService, Task<T>> creator, int number)
+        where TService : class
+        => await services.CreateManyAsync(async (TService service, int _) => await creator(service), number);


style: This method could be simplified using LINQ

greptile-apps · 2024-10-19T09:15:24Z

test/Infrastructure.IntegrationTest/restore.sh

@@ -0,0 +1,112 @@
+#!/usr/bin/env bash
+
+# DIR=$(cd -P "$(dirname "${BASH_SOURCE[0]}")" && pwd)


style: This commented-out line seems unnecessary. Consider removing it if it's not used.

greptile-apps · 2024-10-19T09:15:24Z

test/Infrastructure.IntegrationTest/restore.sh

+get_db() {
+  # Takes
+  DB_NAME=$(echo "$1" | perl -nle"print $& while m{$2=\w+}g" | sed "s/$2=//g")
+  TEST_DB_NAME="${DB_NAME}_int_test"
+  echo "$1" | sed "s/$DB_NAME/$TEST_DB_NAME/"
+}


style: The get_db function is missing a comment explaining what the second parameter ($2) represents.

greptile-apps · 2024-10-19T09:15:25Z

test/Infrastructure.IntegrationTest/restore.sh

+
+if [ ! -z "$SQLITE_CONN_STR" ]; then
+  echo "Making a test database for Sqlite: $SQLITE_CONN_STR"
+  # this makes the assumption it is a file which I think is likely for local dev


logic: This comment assumes SQLite is using a file, which may not always be true. Consider handling in-memory SQLite databases as well.

justindbaur added 11 commits August 24, 2023 09:05

Add Collections & Organizations Tests

6df2577

- Add Helper Script for creating seperated databases for tests

Formatting

012fa80

Fix for Mac

249cf59

Add CollectionCipher Tests

6ae78f3

Update GroupRepository

7742009

- Add Tests - Prefer Method Syntax

Migrate DeviceRepository Tests

079acac

Migrate TaxRepository Tests

653ac16

Migrate PolicyRepository Tests

3182449

Migrate InstallationRepository Tests

4675942

Migrate CollectionRepository Tests

846ee8f

Formatting

1c848de

greptile-apps bot reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-3626] Add Collections & Organizations Tests #68

[PM-3626] Add Collections & Organizations Tests #68

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading

greptile-apps bot left a comment

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

		@@ -148,42 +81,4 @@ public async void GetManyAbilitiesAsync_Works(SqlRepo.OrganizationRepository sql
		list.Concat(await sqlOrganizationRepo.GetManyAbilitiesAsync());

		// The user will be apart of two groups, one that gives them little access to the collection
		// and one that gives them fully access, via our rules, they should be given the full rights.

		@@ -0,0 +1,112 @@
		#!/usr/bin/env bash

		# DIR=$(cd -P "$(dirname "${BASH_SOURCE[0]}")" && pwd)

[PM-3626] Add Collections & Organizations Tests #68

Are you sure you want to change the base?

[PM-3626] Add Collections & Organizations Tests #68

Conversation

lizard-boy commented Oct 19, 2024 • edited by greptile-apps bot Loading

Type of change

Objective

Code changes

Before you submit

Greptile Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading