Azure / azure-cosmos-dotnet-v3

.NET SDK for Azure Cosmos DB for the core SQL API
MIT License
731 stars 489 forks source link

Workaround for unit testing and CosmosLinqQuery<T> #893

Open pseudoramble opened 4 years ago

pseudoramble commented 4 years ago

Is your feature request related to a problem? Please describe. I've written a method to obtain some values using a LinqQueryable and then converting that via the extension method ToFeedIterator() so it can be run asynchronously. It looks roughly like this:

var iterator = this.collection
  .GetItemLinqQueryable<Thingie>()
  .Where(thing => thing.x != "hello")
  .ToFeedIterator();

if (iterator.HasMoreResults)
{
    return (await iterator.ReadNextAsync()).FirstOrDefault();
}

Then in the unit test I make a mocked container return an EnumerableQuery<T> when GetItemLinqQueryable<T>() is called. This is so I can test the actual logic as part of the Linq query. It looks like this:

var samples = /* put some samples here */
var fakeContainer = Substitute.For<Container>();
fakeContainer
  .GetItemLinqQueryable<T>()
  .Returns(new EnumerableQuery<T>(samples));

The net result is hitting the exception about only supporting CosmosLinqQuery instances:

Error Message:
 System.ArgumentOutOfRangeException : ToFeedIterator is only supported on cosmos LINQ query operations
Parameter name: linqQuery
Stack Trace:
   at Microsoft.Azure.Cosmos.Linq.CosmosLinqExtensions.ToFeedIterator[T](IQueryable`1 query)

Describe the solution you'd like I'm open to ideas that let us keep using Linq and writing unit tests.

The main immediate idea I have is exposing an interface or abstract version of CosmosLinuqQuery. There are good reasons to not expose it I think though. So this might not be tractable.

Describe alternatives you've considered I could fake out RequestHandler's in each scenario to work around this. That would make it easier to avoid the other cases where abstract classes or interfaces aren't available. That would be a significant amount of work to do well though. So I'd prefer something else if it's possible.

I could rewrite the code to not use Linq. That might be the route I go to keep making progress. We like using Linq when we can though and it does seem like it's supported officially. So if we could keep doing this that would be nice.

I could skip building unit tests for this section and just focus on integration tests, but that's generally not how we have done things everywhere else in the code. It would be much nicer if I could have support for both like we do today.

Additional context Thanks for the work on this!

j82w commented 4 years ago

This is a problem with the overall linq design with everything being an extension method. It makes it not possible to mock the response.

I do like your idea of exposing CosmosLinqQuery, but it would need to be converted to an abstract class so it works like an interface.

@kirankumarkolli what are your thoughts on doing this?

kirankumarkolli commented 4 years ago

@khdang thoughts?

CaioCavalcanti commented 4 years ago

I'm struggling with the same problem.

I saw this suggestion from @ealsur. But I think it would be nice to have an easier way to work with it.

pseudoramble commented 4 years ago

@CaioCavalcanti - I ended up rewriting the code to use string-based Cosmos queries by swapping out to use GetItemQueryIterator<T>(QueryDefiniton). That will at least let you work around this specific issue if you're in a time crunch.

j82w commented 4 years ago

I did some testing and exposing a CosmosLinqQuery that implements IOrderedQueryable is not that easy to unit test against. The problem is IOrderedQueryable is a rather complex interface. Setting up mocks to use the List properties for the IOrderedQueryable interface works if you immediately call the ToFeedIterator, but if any query operation is done the provider converts it to a List. So it's no longer a CosmosLinqQuery object. Any suggestions? We will gladly review any PRs that are submitted.

ryazan05 commented 4 years ago

I faced the same issue. I ended up creating a wrapper around ToFeedIterator() extension method.

interface ICosmosLinqQuery
{
   FeedIterator<T> GetFeedIterator<T>(IQueryable<T> query);
}

So instead of calling .ToFeedIterator() directly I would call ICosmosLinqQuery implementation with cosmos query. In unit tests I can then mock calls to ICosmosLinqQuery and assert on actual linq query by checking query argument in GetFeedIterator call (using Moq Callback).

pseudoramble commented 4 years ago

@j82w trying to think of some ideas for this. It does seem hard to try and modify the existing CosmosLinqQuery class directly. So far there are only two ideas I can think of:

  1. Something like what @ryazan05 has done where another separate method is invoked on another interface. That's relatively easy, but the extension methods have the convenience of looking like a fluent interface.
  2. Split CosmosLinqQuery<T> into two parts - an interface (or abstract class) where ToFeedIterator is available to override, and the existing class then implements this interface. The extension method could be tweaked to look for an instance of that interface then. This is more complex, but it gives you that chain-ability still and a way to override the interface when needed.

I haven't thought either fully through to see what pitfalls or issues there might be. Just a few ideas.

TechWatching commented 4 years ago

I faced the same issue. I ended up creating a wrapper around ToFeedIterator() extension method.

interface ICosmosLinqQuery
{
   FeedIterator<T> GetFeedIterator<T>(IQueryable<T> query);
}

So instead of calling .ToFeedIterator() directly I would call ICosmosLinqQuery implementation with cosmos query. In unit tests I can then mock calls to ICosmosLinqQuery and assert on actual linq query by checking query argument in GetFeedIterator call (using Moq Callback).

@ryazan05 Would you have a more complete sample by any chance? I have trouble to see how to apply your solution in my context.

          var feedIterator = _myContainer.GetItemLinqQueryable<MyClass>()
                                               .Where(t => t.MyProperty == id)
                                               .ToFeedIterator();
            while (feedIterator.HasMoreResults)
            {
                foreach (var result in await feedIterator.ReadNextAsync())
                {
                    yield return result;
                }
            }
ryazan05 commented 4 years ago

@TechWatching Yes. You will need to implement ICosmosLinqQuery mentioned above. Implementation is just a call to return FeedIterator:

public class MyCosmosLinqQuery : ICosmosLinqQuery
{
   public FeedIterator<T> GetFeedIterator<T>(IQueryable<T> query)
   {
      return query.ToFeedIterator();
   }
}

Then in your code instead of calling ToFeedIterator directly, inject ICosmosLinqQuery implementation and use that to get feedIterator:

var query = _myContainer.GetItemLinqQueryable<MyClass>()
                                               .Where(t => t.MyProperty == id);

var feedIterator = _myCosmosLinqQuery.GetFeedIterator(query);

while (feedIterator.HasMoreResults)
{
   foreach (var result in await feedIterator.ReadNextAsync())
   {
      yield return result;
   }
}

In unit tests you can assert on linq query. Moq example:

...
IQueryable<MyClass> queryResult = null;
var mockCosmosLinqQuery = new Mock<ICosmosLinqQuery>();
mockCosmosLinqQuery.Setup(x => x.GetFeedIterator(...))
   .Callback<IQueryable<MyClass>>(r => queryResult = r)
   .Returns(...);
...
Assert.True(queryResult.All(x => x.MyProperty == expectedId));
GravlLift commented 4 years ago

Does anyone have any ideas on how one might unit test one of the built-in functions like IsDefined? The code I'm testing looks something like this:

Container
  .GetItemLinqQueryable<T>()
  .FirstOrDefault(q => q.Dictionary[StringKey].IsDefined() && q.SomeOtherProperty == "foo");

which of course results in Type check operations can be used in Linq expressions only and are evaluated in Azure CosmosDB server.. If I swap that IsDefined call in the source for IDictionary's ContainsKey, the test works but then the actual Azure call fails.

tillig commented 3 years ago

Running into this exact thing now. I'm wondering if there'd be a way to support ToFeedIterator() on any queryable rather than throw an exception, where if it's not a special Cosmos DB queryable then it'd just get a sort of fixed/dummy value instead. Like, maybe the non-Cosmos iterator would just say, "There's just this one set of results." It isn't super awesome to have to break out of LINQ just so I can test things.

danielwcr commented 3 years ago

Same old issue with ms sdks. I wonder if ms will ever be able to provide a testable client.

markbeij commented 2 years ago

Any update on this?

andyfurniss4 commented 2 years ago

Would also like an update on this. Been a while since this was raised...

PacoCreaCodigo commented 2 years ago

Any update? feels weird to create an interface...

samysammour commented 2 years ago

I had a slightly different approach for the NSubstitute in case anyone needs it.

Creating the method


public class CosmosDbLinqQuery : ICosmosDbLinqQuery
{
    public FeedIterator<TEntity> GetFeedIterator<TEntity>(IQueryable<TEntity> source)
    {
        return source.ToFeedIterator();
    }
}

public interface ICosmosDbLinqQuery
{
    FeedIterator<TEntity> GetFeedIterator<TEntity>(IQueryable<TEntity> source)
}

I have created an instance of the interface in the constructor to not extend my Factory but mark it as a virtual to be overwritten

public CosmosDbRepository()
{
    this.CosmosDbLinqQuery = new CosmosDbLinqQuery();
}

// For testing purposes
protected virtual ICosmosDbLinqQuery CosmosDbLinqQuery { get; set; }

The usage:

var iterator = this.CosmosDbLinqQuery.GetFeedIterator(query);

I have created a test repository that inherits the generic repository and replace the CosmosDbLinqQuery method with mock

public class TestCosmosDbRepository<TEntity> : CosmosDbRepository<TEntity>, ICosmosDbRepository<TEntity>
{
    private ICosmosDbLinqQuery cosmosDbLinqQuery;
    public TestCosmosDbRepository() : base()
    {
    }

    protected override ICosmosDbLinqQuery CosmosDbLinqQuery
    {
        get => this.cosmosDbLinqQuery;
        set
        {
            var linqQuery = Substitute.For<ICosmosDbLinqQuery>();
            linqQuery.GetFeedIterator(Arg.Any<IQueryable<Stub>>())
                .Returns(x => this.GetFeedIteratorMock((IQueryable<Stub>)x[0]));
            this.cosmosDbLinqQuery = linqQuery;
        }

    }

    private FeedIterator<Stub> GetFeedIteratorMock(IQueryable<Stub> list)
    {
        var feedResponse = Substitute.For<FeedResponse<Stub>>();
        var feedIterator = Substitute.For<FeedIterator<Stub>>();

        feedResponse.Resource.Returns(list.ToList());
        feedIterator.HasMoreResults.Returns(true);
        feedIterator.ReadNextAsync(cancellationToken: default).ReturnsForAnyArgs(feedResponse)
            .AndDoes(x => feedIterator.HasMoreResults.Returns(false));
        return feedIterator;
    }
}

The testing will work like a charm:

var repo = new TestCosmosDbRepository<Stub>();
....
// the container mock depends on the approach but is accessible like this
this.container.GetItemLinqQueryable<Stub>().ReturnsForAnyArgs(list.AsQueryable());

Happy coding!

Zhorian commented 1 year ago

From my playing around with this the most painful aspect is the feed iterator. Could you not just wrap it in a service and then mock said service?

namespace Triggers.Services
{
    using Microsoft.Azure.Cosmos;
    using Microsoft.Azure.Cosmos.Linq;
    using System.Collections.Generic;
    using System.Linq;

    public static class CosmosFeedIteratorExtensions
    {
        public static async IAsyncEnumerable<T> ToAsyncEnumerable<T>(this FeedIterator<T> feedIterator)
        {
            while (feedIterator.HasMoreResults)
            {
                foreach (var responseItem in await feedIterator.ReadNextAsync())
                {
                    yield return responseItem;
                }
            }
        }
    }

    public interface ICosmosQueryService
    {
        Task<List<T>> QueryToList<T>(IOrderedQueryable<T> queryable);
    }

    public class CosmosQueryService : ICosmosQueryService
    {
        public async Task<List<T>> QueryToList<T>(IOrderedQueryable<T> queryable)
        {
            var feed = queryable.ToFeedIterator();
            return await feed.ToAsyncEnumerable().ToListAsync();
        }
    }
}

Then setup up the test like this?

            var queryable = data.AsQueryable().OrderBy(x => x.Id);

            this.mockCosmosClientFactory.Setup(x => x.CreateCosmosClient()).Returns(this.mockCosmosClient.Object);
            this.mockCosmosClient.Setup(x => x.GetContainer(CosmosNames.OtaDatabase, CosmosNames.OtaUpdateMetadata)).Returns(this.mockContainer.Object);
            this.mockContainer.Setup(x => x.GetItemLinqQueryable<UpdateMetadataModel>(false, null, null, null)).Returns(queryable);
            this.mockCosmosQueryService.Setup(x => x.QueryToList(It.IsAny<IOrderedQueryable<UpdateMetadataModel>>())).ReturnsAsync(data);
andrew-lis commented 6 months ago

The biggest problem is that you can't even create a stub for it (I don't want to use mocks). I can create a stub/fake implementation for IOrderedQueryable in tests, but ToFeedIterator() is an evil extension method that hardcodes: ((query as CosmosLinqQuery<T>) and since CosmosLinqQuery is a sealed class, I can't inherit from it.

Why a such important interface is not TDD friendly?

It seems that the only options left are: