feat(Storage): Add filter to ListObjectOptions and integration tests for object context#11
feat(Storage): Add filter to ListObjectOptions and integration tests for object context#11mahendra-google wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello @mahendra-google, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the Google Cloud Storage client library by introducing a suite of new integration tests specifically for the object context feature. These tests validate the behavior of object contexts across various operations, including copying, retrieving, patching, and uploading objects, ensuring the feature functions as expected in real-world scenarios. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a good set of integration tests for the new object context feature, covering copy, get, patch, update, and upload operations. The test cases are logical and cover important scenarios. I have identified a couple of areas where the test assertions can be improved for correctness and robustness. One test contains an incorrect assertion that will likely cause a runtime error, and another could be strengthened to more thoroughly validate the update functionality. My specific comments provide code suggestions to address these points.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces new integration tests for the object context feature across various Storage operations like CopyObject, GetObject, ListObjects, PatchObject, and UploadObject. The changes primarily involve adding new test methods to verify the functionality of object contexts, including setting, retrieving, updating, and filtering by custom contexts. The new tests enhance the coverage for this feature, ensuring its correct behavior in different scenarios.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => | ||
| x.Contexts == null && | ||
| x.Contexts.Custom == null && | ||
| !x.Contexts.Custom.ContainsKey(filteredCustom.Keys.FirstOrDefault())).ToList(); |
There was a problem hiding this comment.
The Where clause in ListObjectsWithoutContextKey contains a logical error. If x.Contexts is null, accessing x.Contexts.Custom will result in a NullReferenceException. Also, x.Contexts.Custom == null and !x.Contexts.Custom.ContainsKey(...) are contradictory if x.Contexts.Custom is null. The logic needs to be re-evaluated to correctly filter objects that do not contain a specific context key.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x =>
x.Contexts == null ||
x.Contexts.Custom == null ||
!x.Contexts.Custom.ContainsKey(filteredCustom.Keys.FirstOrDefault())).ToList();|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts == filteredContext).ToList(); |
There was a problem hiding this comment.
The Where clause for filtering objects by context key-value pair uses x.Contexts == filteredContext. This comparison might not work as expected for complex objects like Object.ContextsData and Dictionary<string, ObjectCustomContextPayload> in C#. By default, == for reference types checks for reference equality, not value equality. You should compare the Custom dictionaries explicitly.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts?.Custom?.SequenceEqual(filteredContext.Custom) == true).ToList();|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts != filteredContext).ToList(); |
There was a problem hiding this comment.
Similar to the previous comment, the Where clause x.Contexts != filteredContext will perform reference inequality check, which is likely not the intended behavior for comparing object contexts. You should explicitly compare the Custom dictionaries for inequality.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts?.Custom?.SequenceEqual(filteredContext.Custom) != true).ToList();| { | ||
| Assert.Equal(obj.Contexts.Custom, filteredCustom); |
There was a problem hiding this comment.
The assertion Assert.Equal(obj.Contexts.Custom, filteredCustom); in ListObjectsWithoutContextKey seems incorrect given the test name "ListObjectsWithoutContextKey". This test should assert that the objects do not contain the specified context key, or that the context is null/empty, not that they are equal to filteredCustom.
Assert.False(obj.Contexts?.Custom?.ContainsKey(filteredCustom.Keys.FirstOrDefault()) == true);
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces new integration tests for the object context feature across various Storage operations like Copy, Get, List, Patch, and Upload. The added tests cover important scenarios such as copying objects with contexts, retrieving objects with contexts, listing objects with and without context filters, patching contexts (clearing, removing individual, modifying, and adding), and uploading objects with contexts. Overall, the new tests are a valuable addition, but some assertions could be more robust or logically correct to prevent potential false positives or incorrect test outcomes.
| !targetKeysSet.SetEquals(x.Contexts.Custom.Keys)).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(obj.Contexts.Custom.Keys, filteredCustom.Keys); |
There was a problem hiding this comment.
There appears to be a logical error in this assertion. The Where clause filters for objects where the key set is not equal (!targetKeysSet.SetEquals(...)). Therefore, asserting that obj.Contexts.Custom.Keys is equal to filteredCustom.Keys (which is targetKeysSet) contradicts the filter's intent. This assertion should likely be Assert.False or Assert.NotEqual to match the filtering logic.
Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(filteredCustom.Keys));|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts == filteredContext).ToList(); |
There was a problem hiding this comment.
Comparing x.Contexts == filteredContext directly for Object.ContextsData (a class) will perform a reference equality check, not a value equality check. This means the filter will likely not work as intended unless Object.ContextsData overrides Equals and GetHashCode for deep comparison, which is uncommon for generated data classes. You'll need to compare the properties of Contexts explicitly, for example, by comparing the Custom dictionaries' contents.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts == filteredContext).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(obj.Contexts.Custom, filteredCustom); |
There was a problem hiding this comment.
Similar to the Where clause, Assert.Equal(obj.Contexts.Custom, filteredCustom); for dictionaries will typically perform a reference equality check or an order-dependent comparison, which might not be robust for dictionary contents. It's better to explicitly compare the contents of the dictionaries, for example, by checking if they have the same keys and if the values for each key are equal.
|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts != filteredContext).ToList(); |
There was a problem hiding this comment.
Similar to the previous ListObjectsFilteredByContextKeyValuePair test, comparing x.Contexts != filteredContext directly for Object.ContextsData (a class) will perform a reference inequality check, not a value inequality check. This means the filter will likely not work as intended. You'll need to compare the properties of Contexts explicitly.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts != filteredContext).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.NotEqual(obj.Contexts, filteredContext); |
There was a problem hiding this comment.
Similar to the Where clause, Assert.NotEqual(obj.Contexts, filteredContext); for Object.ContextsData will typically perform a reference inequality check. For a robust test, you should explicitly compare the contents of the Contexts objects to ensure they are indeed different as expected by the filter.
| _fixture.SingleVersionBucket, destination.Name, | ||
| _fixture.MultiVersionBucket, destination.Name); | ||
| Object fetched = _fixture.Client.GetObject(_fixture.MultiVersionBucket, destination.Name); | ||
| Assert.Equal(result.Contexts.Custom.Keys, fetched.Contexts.Custom.Keys); |
There was a problem hiding this comment.
When comparing collections like dictionary keys, it's generally more robust to use SequenceEqual from LINQ, especially since the order of keys in a dictionary is not guaranteed. This ensures that the content of the key collections is compared, not just their references or a potentially order-dependent equality check.
Assert.True(result.Contexts.Custom.Keys.SequenceEqual(fetched.Contexts.Custom.Keys));| var obj = _fixture.Client.GetObject(_fixture.MultiVersionBucket, destination.Name); | ||
| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); |
There was a problem hiding this comment.
The current assertion only checks the count of custom contexts. To ensure the object context was correctly retrieved, you should also assert that the actual key-value pairs within obj.Contexts.Custom match those in result.Contexts.Custom. This provides a more comprehensive validation of the retrieved context data.
| foreach (var obj in objects) | ||
| { | ||
| Assert.NotNull(obj.Name); | ||
| Assert.NotNull(obj.Contexts); |
There was a problem hiding this comment.
While Assert.NotNull(obj.Contexts); confirms the presence of contexts, it doesn't verify that the content of the contexts is as expected. For a complete test, consider adding assertions to check the actual key-value pairs within obj.Contexts.Custom against the custom dictionary used for upload.
| targetKeysSet.SetEquals(x.Contexts.Custom.Keys)).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(obj.Contexts.Custom.Keys, filteredCustom.Keys); |
There was a problem hiding this comment.
When comparing collections like dictionary keys, it's generally more robust to use SequenceEqual from LINQ, especially since the order of keys in a dictionary is not guaranteed. This ensures that the content of the key collections is compared, not just their references or a potentially order-dependent equality check.
Assert.True(obj.Contexts.Custom.Keys.SequenceEqual(filteredCustom.Keys));| var result = _fixture.Client.UploadObject(destination, source); | ||
| Assert.Equal(destination.Name, result.Name); | ||
| Assert.Equal(destination.Bucket, result.Bucket); | ||
| Assert.Equal(destination.Contexts.Custom.Keys, result.Contexts.Custom.Keys); |
There was a problem hiding this comment.
When comparing collections like dictionary keys, it's generally more robust to use SequenceEqual from LINQ, especially since the order of keys in a dictionary is not guaranteed. This ensures that the content of the key collections is compared, not just their references or a potentially order-dependent equality check.
Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(result.Contexts.Custom.Keys));
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature, covering create, read, update, delete, and list operations. This is a valuable addition for ensuring the feature's correctness. However, I've identified several issues within the new tests, particularly in ListObjectsTest.cs, where the filtering logic is incorrect due to improper object comparisons. Additionally, some tests rely on hardcoded strings in assertions, which makes them brittle. I've provided specific feedback and code suggestions to improve the robustness and maintainability of these tests.
|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts == filteredContext).ToList(); |
There was a problem hiding this comment.
The filtering logic x.Contexts == filteredContext uses reference equality for the ContextsData object. Since the ListObjects method returns new object instances, this comparison will almost certainly always be false, even if the contents are identical. The test is therefore not functioning as intended.
To fix this, you should compare the contents of the context data, for example by comparing the Custom dictionaries. Also, the test name is misleading as it implies server-side filtering, whereas the filtering is happening on the client side.
|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts != filteredContext).ToList(); |
There was a problem hiding this comment.
The filtering logic x.Contexts != filteredContext uses reference inequality. For different object instances, this will almost always be true, regardless of their content. This means the test is not correctly filtering out objects with matching context data, and is not testing what its name implies.
To fix this, the comparison should be based on the content of the context data, not the object reference.
| !targetKeysSet.SetEquals(x.Contexts.Custom.Keys)).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(obj.Contexts.Custom.Keys, filteredCustom.Keys); |
There was a problem hiding this comment.
There is a logical contradiction in this test. The Where clause on line 274 filters for objects where the custom context keys are not equal to targetKeysSet. However, the assertion inside the loop then checks if the keys are equal. This will cause the test to fail, and it does not correctly verify the filtering logic.
Assert.False(targetKeysSet.SetEquals(obj.Contexts.Custom.Keys));| var obj = _fixture.Client.GetObject(_fixture.MultiVersionBucket, destination.Name); | ||
| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); |
There was a problem hiding this comment.
The current assertion only checks if the number of custom context entries is the same. This is a weak assertion as it doesn't verify the actual content (keys and values). A more robust test would compare the fetched context with the original data to ensure they are identical.
Assert.Equal(custom, obj.Contexts.Custom);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds valuable integration tests for the new object context feature across various operations like copy, get, list, patch, update, and upload. The tests are generally well-written and cover important scenarios. However, I've identified a few issues in the new tests within GetObjectTest.cs and ListObjectsTest.cs. Some tests in ListObjectsTest.cs have logical flaws that cause them to be ineffective, either by passing vacuously or having contradictory assertions. My specific comments provide details for correction.
|
|
||
| var source = GenerateData(100); | ||
| var result = _fixture.Client.UploadObject(destination, source); | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => x.Contexts == filteredContext).ToList(); |
There was a problem hiding this comment.
The Where(x => x.Contexts == filteredContext) clause uses reference equality on Object.ContextsData instances. Because filteredContext is a new object, this comparison will always be false, resulting in an empty objects list. The test then passes without executing any assertions inside the loop. To fix this, you need to perform a value-based comparison of the context data.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x => | ||
| x.Contexts != null && | ||
| !targetKeysSet.SetEquals(x.Contexts.Custom.Keys)).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(obj.Contexts.Custom.Keys, filteredCustom.Keys); | ||
| } |
There was a problem hiding this comment.
The assertion on line 277 contradicts the filter condition. The Where clause selects objects where the context keys do not match targetKeysSet. However, the assertion Assert.Equal(obj.Contexts.Custom.Keys, filteredCustom.Keys) checks if they do match. This test is logically flawed and will fail if it correctly selects an object that meets the filter criteria.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).Where(x =>
x.Contexts != null &&
!targetKeysSet.SetEquals(x.Contexts.Custom.Keys)).ToList();
foreach (var obj in objects)
{
Assert.False(targetKeysSet.SetEquals(obj.Contexts.Custom.Keys));
}| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); | ||
| Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(result.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
This assertion checks the result of the UploadObject call (result) rather than the GetObject call (obj). To ensure the test is validating the behavior of GetObject as its name implies, the assertion should compare against the obj variable.
Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(obj.Contexts.Custom.Keys));
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new integration test cases for the object context feature across various Storage operations, including CopyObject, GetObject, ListObjects, PatchObject, and UploadObject. The new tests cover important scenarios like copying contexts, retrieving contexts, listing objects by context filters, and modifying contexts via patch and update operations. My feedback focuses on enhancing the robustness and correctness of some of these new tests, specifically by improving assertions to ensure complete comparison of object contexts (both keys and values) and correcting the construction of filter strings in list operations.
|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $"contexts.{custom.Keys} = {custom.Values}"; |
There was a problem hiding this comment.
custom.Keys and custom.Values are collections. Directly interpolating them into the filter string will insert their type names, not the actual key or value. Since the custom dictionary is initialized with a single entry, Single() should be used to extract the key and value for the filter string.
string filter = $"contexts.{custom.Keys.Single()} = {custom.Values.Single().Value}";|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $"-contexts.{custom.Keys}:*"; |
There was a problem hiding this comment.
custom.Keys is a collection. Directly interpolating it into the filter string will insert its type name, not the actual key. Since the custom dictionary is initialized with a single entry, Single() should be used to extract the key for the filter string.
string filter = $"-contexts.{custom.Keys.Single()}:*";|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $"-contexts.{custom.Keys} = {custom.Values}"; |
There was a problem hiding this comment.
custom.Keys and custom.Values are collections. Directly interpolating them into the filter string will insert their type names, not the actual key or value. Since the custom dictionary is initialized with a single entry, Single() should be used to extract the key and value for the filter string.
string filter = $"-contexts.{custom.Keys.Single()} = {custom.Values.Single().Value}";|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $"contexts.{custom.Keys}:*"; |
There was a problem hiding this comment.
custom.Keys is a collection. Directly interpolating it into the filter string will insert its type name, not the actual key. Since the custom dictionary is initialized with a single entry, Single() should be used to extract the key for the filter string.
string filter = $"contexts.{custom.Keys.Single()}:*";| if (obj.Contexts != null) | ||
| { | ||
| Assert.NotNull(obj.Contexts); | ||
| } |
There was a problem hiding this comment.
The if (obj.Contexts != null) condition is redundant here. If obj.Contexts is null, Assert.NotNull(obj.Contexts) will fail. If it's not null, the if condition is met, and Assert.NotNull on a non-null object is unnecessary.
Assert.NotNull(obj.Contexts);| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); | ||
| Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(obj.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
This assertion only checks if the keys of the custom contexts are equal. For a complete test of context retrieval, the values associated with these keys should also be compared.
Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(obj.Contexts.Custom.Keys));
Assert.True(destination.Contexts.Custom.All(pair => obj.Contexts.Custom.ContainsKey(pair.Key) && obj.Contexts.Custom[pair.Key].Value == pair.Value.Value));| { | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.NotEqual(obj.Contexts.Custom.Keys, custom.Keys); |
There was a problem hiding this comment.
Assert.NotEqual on collections typically checks for reference equality. For asserting that the content of two collections is not equal, SequenceEqual can be used with a False assertion.
Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(custom.Keys));| Assert.True(fetched.Contexts.Custom.ContainsKey(newCustom.Keys.Single())); | ||
| Assert.False(fetched.Contexts.Custom.ContainsKey(custom.Keys.Single())); |
There was a problem hiding this comment.
The assertions correctly verify the presence of the new key and absence of the old key. To fully confirm the override behavior, it would be beneficial to also assert the value of the newly added context.
Assert.True(fetched.Contexts.Custom.ContainsKey(newCustom.Keys.Single()));
Assert.Equal(newCustom.Values.Single().Value, fetched.Contexts.Custom[newCustom.Keys.Single()].Value);
Assert.False(fetched.Contexts.Custom.ContainsKey(custom.Keys.Single()));| { | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.NotEqual(obj.Contexts.Custom.Keys, custom.Keys); |
There was a problem hiding this comment.
Assert.NotEqual on collections typically checks for reference equality. For asserting that the content of two collections is not equal, SequenceEqual can be used with a False assertion.
Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(custom.Keys));| var result = _fixture.Client.UploadObject(destination, source); | ||
| Assert.Equal(destination.Name, result.Name); | ||
| Assert.Equal(destination.Bucket, result.Bucket); | ||
| Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(result.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
This assertion only verifies that the keys of the custom contexts are equal. For a complete test of context uploading, the values associated with these keys should also be checked to ensure the entire context is uploaded correctly.
Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(result.Contexts.Custom.Keys));
Assert.True(destination.Contexts.Custom.All(pair => result.Contexts.Custom.ContainsKey(pair.Key) && result.Contexts.Custom[pair.Key].Value == pair.Value.Value));
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature. The coverage is good, spanning copy, get, list, patch, update, and upload operations. I've identified several issues within the new tests, including a few critical bugs where tests are broken due to incorrect filter string construction or flawed assertion logic. There are also some cases of flaky or brittle assertions that rely on dictionary key ordering or use Single() where it's not robust. Additionally, I've made some suggestions to improve code maintainability by removing hardcoded strings and redundant code. Please see the detailed comments for specific suggestions.
|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $@"-contexts.""{custom.Keys}""=""{custom.Values}"""; |
There was a problem hiding this comment.
There's a bug in how the filter string is constructed. custom.Keys and custom.Values are collections, and calling ToString() on them will not produce the desired key and value strings for the filter. You should use Single() to get the individual key and value.
string filter = $@"-contexts.""{custom.Keys.Single()}""=""{custom.Values.Single().Value}""";| foreach (var obj in objects) | ||
| { | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(custom.Keys)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The assertion logic here is incorrect. The test should verify that the object with the specified context key is excluded from the results. The current loop makes a flawed check on the objects that are returned. The correct approach is to assert that the object created for this test is not present in the result list.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList();
Assert.DoesNotContain(objects, o => o.Name == destination.Name);|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $@"-contexts.""{custom.Keys}"":*"; |
There was a problem hiding this comment.
The filter string is constructed incorrectly. custom.Keys is a collection, and its ToString() method won't produce the key string needed for the filter. You should use Single() to retrieve the key.
string filter = $@"-contexts.""{custom.Keys.Single()}"":*";| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(custom.Keys)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The assertion logic for this test is incorrect. The goal of a negative filter is to ensure the object matching the filter is not included in the results. The current loop iterates through the results and makes a flawed assertion about their contexts. A much simpler and more correct way to test this is to assert that the object created for this test does not exist in the list of returned objects.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList();
Assert.DoesNotContain(objects, o => o.Name == destination.Name);| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.NotNull(obj.Name); | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.NotNull(obj.Contexts); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current test logic is not very specific. It lists all objects in the bucket and performs a weak check. A more robust approach would be to specifically find the object created in the test and verify its contexts are correct. This ensures the test is precise and not affected by other objects in the bucket.
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, prefix: destination.Name).ToList();
var obj = Assert.Single(objects);
Assert.NotNull(obj.Contexts);
Assert.True(obj.Contexts.Custom.ContainsKey(custom.Keys.Single()));
Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value);| Assert.True(fetched.Contexts.Custom.ContainsKey(newCustom.Keys.Single())); | ||
| Assert.False(fetched.Contexts.Custom.ContainsKey(custom.Keys.Single())); | ||
| Assert.Equal(newCustom.Values.Single().Value, fetched.Contexts.Custom[newCustom.Keys.Single()].Value); | ||
| Assert.False(fetched.Contexts.Custom.ContainsKey(custom.Keys.Single())); |
There was a problem hiding this comment.
This assertion is a duplicate of the one on line 185. It can be removed to avoid redundancy.
|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal("AbcñΩ🚀", updated.Contexts.Custom["AñΩ🚀"].Value); |
There was a problem hiding this comment.
Using hardcoded string literals for both the expected value and the dictionary key in the assertion makes the test harder to maintain. It's better to use the variables from the test setup (modifiedCustom) to make the assertion more readable and robust against changes in the test data.
Assert.Equal(modifiedCustom.Values.Single().Value, updated.Contexts.Custom[modifiedCustom.Keys.Single()].Value);|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal(2, updated.Contexts.Custom.Keys.Count); |
There was a problem hiding this comment.
While asserting the count is 2 is a good start, the test could be made more robust by also asserting that both the original and the newly added context keys are present in the updated object. This would more thoroughly verify the patch operation's additive behavior.
Assert.Equal(2, updated.Contexts.Custom.Keys.Count);
Assert.True(updated.Contexts.Custom.ContainsKey(custom.Keys.Single()));
Assert.True(updated.Contexts.Custom.ContainsKey(modifiedCustom.Keys.Single()));| Assert.Equal(modifiedCustom.Count, updated.Contexts.Custom.Count); | ||
| Assert.Equal(modifiedCustom["AñΩ🚀"].Value, updated.Contexts.Custom["AñΩ🚀"].Value); | ||
| Assert.Equal(modifiedCustom["CAñΩ🚀"].Value, updated.Contexts.Custom["CAñΩ🚀"].Value); | ||
| Assert.Equal(modifiedCustom["CBñΩ🚀"].Value, updated.Contexts.Custom["CBñΩ🚀"].Value); |
There was a problem hiding this comment.
These assertions can be made more maintainable and concise. Instead of having a separate assertion with a hardcoded key for each item, you can iterate over the modifiedCustom dictionary. This makes the test more robust if you decide to add or change contexts in the test data.
Assert.Equal(modifiedCustom.Count, updated.Contexts.Custom.Count);
foreach (var pair in modifiedCustom)
{
Assert.Equal(pair.Value.Value, updated.Contexts.Custom[pair.Key].Value);
}| var result = _fixture.Client.UploadObject(destination, source); | ||
| Assert.Equal(destination.Name, result.Name); | ||
| Assert.Equal(destination.Bucket, result.Bucket); | ||
| Assert.True(destination.Contexts.Custom.Keys.Single().SequenceEqual(result.Contexts.Custom.Keys.Single())); |
There was a problem hiding this comment.
Using SequenceEqual to compare two strings (which is what Keys.Single() returns) is unconventional and less clear than using Assert.Equal. For better readability, it's recommended to use Assert.Equal for string comparisons.
Assert.Equal(destination.Contexts.Custom.Keys.Single(), result.Contexts.Custom.Keys.Single());
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature. The changes are well-structured and cover various API endpoints. I've identified a few areas where the test assertions can be made more robust and resilient to future changes, along with some minor code style improvements. Overall, this is a solid contribution to improving test coverage.
|
|
||
| var source = GenerateData(100); | ||
| _fixture.Client.UploadObject(destination, source); | ||
| string filter = $@"-contexts.""{custom.Keys.Single()}""=""{custom.Values.Single()}"""; |
There was a problem hiding this comment.
The filter string construction is slightly incorrect. custom.Values.Single() will return an ObjectCustomContextPayload, not the string value itself. You should access the .Value property.
string filter = $@"-contexts.""{custom.Keys.Single()}""=""{custom.Values.Single().Value}""";| foreach (var obj in objects) | ||
| { | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(custom.Keys)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This assertion is weak. It only checks that if an object has contexts, they don't match the one from the object we created. A much stronger and simpler assertion would be to verify that the object we created is not present in the returned list at all.
Assert.DoesNotContain(objects, obj => obj.Name == destination.Name);| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); | ||
| Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(obj.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
This assertion checking for key sequence equality is redundant. The assertion on line 119 checks for the same number of items, and the assertion on line 121 checks that all keys and values from the original destination context exist in the fetched obj context. Together, they already confirm that the key sets are identical. Relying on key order with SequenceEqual can also make tests brittle if the service doesn't guarantee order.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.NotNull(obj.Name); | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.NotNull(obj.Contexts); | ||
| } | ||
| } |
There was a problem hiding this comment.
The assertions in this test are quite weak. It iterates through all objects in the bucket but doesn't specifically check the object that was just uploaded. The check if (obj.Contexts != null) { Assert.NotNull(obj.Contexts); } is also redundant. A better test would be to find the newly created object in the list (e.g., by filtering with a prefix) and assert that its context is what you expect.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.True(obj.Contexts.Custom.Keys.Single().SequenceEqual(custom.Keys.Single())); |
There was a problem hiding this comment.
Using SequenceEqual to compare two strings (the single keys from the dictionaries) is unconventional and can be confusing. Assert.Equal would be more idiomatic for string comparison. Additionally, relying on Single() makes the test fragile if the test data changes to include more than one context.
Assert.Equal(custom.Keys.Single(), obj.Contexts.Custom.Keys.Single());| foreach (var obj in objects) | ||
| { | ||
| if (obj.Contexts != null) | ||
| { | ||
| Assert.False(obj.Contexts.Custom.Keys.SequenceEqual(custom.Keys)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This assertion is weak. It only checks that if an object has contexts, they don't match the one from the object we created. A much stronger and simpler assertion would be to verify that the object we created is not present in the returned list at all.
Assert.DoesNotContain(objects, obj => obj.Name == destination.Name);|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal("AbcñΩ🚀", updated.Contexts.Custom["AñΩ🚀"].Value); |
There was a problem hiding this comment.
The hardcoded Unicode string "AbcñΩ🚀" makes the assertion less readable and maintainable. It would be better to use the modifiedCustom dictionary to retrieve the expected value, which makes the test's intent clearer and less prone to typos.
Assert.Equal(modifiedCustom["AñΩ🚀"].Value, updated.Contexts.Custom["AñΩ🚀"].Value);| Assert.Equal(modifiedCustom["AñΩ🚀"].Value, updated.Contexts.Custom["AñΩ🚀"].Value); | ||
| Assert.Equal(modifiedCustom["CAñΩ🚀"].Value, updated.Contexts.Custom["CAñΩ🚀"].Value); | ||
| Assert.Equal(modifiedCustom["CBñΩ🚀"].Value, updated.Contexts.Custom["CBñΩ🚀"].Value); |
There was a problem hiding this comment.
These assertions are repetitive. Using a loop to iterate over the expected contexts would make the test more concise and easier to maintain if more contexts are added in the future.
foreach (var expectedContext in modifiedCustom)
{
Assert.Equal(expectedContext.Value.Value, updated.Contexts.Custom[expectedContext.Key].Value);
}| Assert.True(destination.Contexts.Custom.Keys.Single().SequenceEqual(result.Contexts.Custom.Keys.Single())); | ||
| Assert.Equal(destination.Contexts.Custom.Values.Single().Value, result.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
These assertions are a bit fragile and hard to read. Using Single() can cause tests to fail if the test data is ever updated to include more than one context. Also, using SequenceEqual to compare single strings is unconventional; Assert.Equal is more appropriate. A more robust approach would be to check the count and then assert the key and value directly.
var expectedContext = Assert.Single(destination.Contexts.Custom);
var actualContext = Assert.Single(result.Contexts.Custom);
Assert.Equal(expectedContext.Key, actualContext.Key);
Assert.Equal(expectedContext.Value.Value, actualContext.Value.Value);| EndOffset = "end", | ||
| MatchGlob = "a/*.txt" | ||
| MatchGlob = "a/*.txt", | ||
| Filter = "contexts.\"key\":*\"" |
There was a problem hiding this comment.
The filter string contexts."key":*" appears to have a typo with an extra double quote at the end. Based on the filter syntax, it should likely be contexts."key":*.
Filter = "contexts.\"key\":*"
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature, covering various operations like copy, get, list, patch, update, and upload. The changes are well-structured. My review focuses on improving the robustness and correctness of the test assertions. I've found several instances where tests use fragile patterns like .Single() or incorrect methods for comparison like SequenceEqual on strings. I've provided specific suggestions to make the tests more reliable and maintainable.
| Assert.True(obj.Contexts.Custom.Keys.Single().SequenceEqual(custom.Keys.Single())); | ||
| Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
There are a couple of issues here:
SequenceEqualon a string (custom.Keys.Single()) compares it character by character. This is likely not the intended comparison and is a bug in the test. You should useAssert.Equalfor string comparison or check for key existence.- Using
.Single()makes the test fragile.
A more robust and correct way to assert this would be to get the single key-value pair from the custom dictionary and then check for its presence and correct value in the obj.Contexts.Custom dictionary.
var customPair = Assert.Single(custom);
Assert.True(obj.Contexts.Custom.ContainsKey(customPair.Key));
Assert.Equal(customPair.Value.Value, obj.Contexts.Custom[customPair.Key].Value);| Assert.True(obj.Contexts.Custom.Keys.Single().SequenceEqual(custom.Keys.Single())); | ||
| Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
Similar to another test in this file, there are issues with these assertions:
SequenceEqualon a string (custom.Keys.Single()) is incorrect for comparing keys. It compares character sequences. You should check for key equality or existence.- Using
.Single()makes the test fragile to changes in test data.
Please refactor to use a more robust assertion pattern.
var customPair = Assert.Single(custom);
Assert.True(obj.Contexts.Custom.ContainsKey(customPair.Key));
Assert.Equal(customPair.Value.Value, obj.Contexts.Custom[customPair.Key].Value);| Assert.True(destination.Contexts.Custom.Keys.Single().SequenceEqual(result.Contexts.Custom.Keys.Single())); | ||
| Assert.Equal(destination.Contexts.Custom.Values.Single().Value, result.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
There are a couple of issues with these assertions:
SequenceEqualon a string (custom.Keys.Single()) compares it character by character. This is a bug in the test logic. You should be checking for key equality.- Using
.Single()onKeysandValuescollections separately makes the test fragile and harder to read.
A more robust approach is to get the single KeyValuePair from the dictionary and then check for its presence and value in the result.
var customPair = Assert.Single(destination.Contexts.Custom);
Assert.NotNull(result.Contexts?.Custom);
Assert.True(result.Contexts.Custom.TryGetValue(customPair.Key, out var resultPayload));
Assert.Equal(customPair.Value.Value, resultPayload.Value);
Assert.Equal(1, result.Contexts.Custom.Count);| _fixture.SingleVersionBucket, destination.Name, | ||
| _fixture.MultiVersionBucket, destination.Name); | ||
| Object fetched = _fixture.Client.GetObject(_fixture.MultiVersionBucket, destination.Name); | ||
| Assert.True(result.Contexts.Custom.Keys.SequenceEqual(fetched.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
The assertion result.Contexts.Custom.Keys.SequenceEqual(fetched.Contexts.Custom.Keys) is not robust. It only checks that the keys are the same and in the same order, but it doesn't verify the values of the contexts. Additionally, relying on key order for dictionaries can be brittle.
A more thorough check would be to verify the counts and then check that each key-value pair from the source exists in the destination.
Assert.Equal(result.Contexts.Custom.Count, fetched.Contexts.Custom.Count);
Assert.All(result.Contexts.Custom, kvp =>
{
Assert.True(fetched.Contexts.Custom.TryGetValue(kvp.Key, out var fetchedValue));
Assert.Equal(kvp.Value.Value, fetchedValue.Value);
});| Assert.True(fetched.Contexts.Custom.ContainsKey(newCustom.Keys.Single())); | ||
| Assert.Equal(newCustom.Values.Single().Value, fetched.Contexts.Custom[newCustom.Keys.Single()].Value); | ||
| Assert.False(fetched.Contexts.Custom.ContainsKey(custom.Keys.Single())); |
There was a problem hiding this comment.
Using .Single() on the keys and values collections makes the test fragile. If the test data is changed in the future to include more than one item, these assertions will break. It's more robust and readable to use Assert.Single on the dictionary to get the KeyValuePair and then use its properties for assertions.
var newCustomPair = Assert.Single(newCustom);
var oldCustomPair = Assert.Single(custom);
Assert.True(fetched.Contexts.Custom.ContainsKey(newCustomPair.Key));
Assert.Equal(newCustomPair.Value.Value, fetched.Contexts.Custom[newCustomPair.Key].Value);
Assert.False(fetched.Contexts.Custom.ContainsKey(oldCustomPair.Key));| string filter = $@"contexts.""{custom.Keys.Single()}""=""{custom[custom.Keys.Single()].Value}"""; | ||
| var options = new ListObjectsOptions { Filter = filter }; |
There was a problem hiding this comment.
The construction of the filter string using .Single() is fragile. If the test data in the custom dictionary changes, this could break. It's better to extract the key-value pair in a more robust way to build the filter string.
var customPair = Assert.Single(custom);
var options = new ListObjectsOptions { Filter = $@"contexts."{customPair.Key}"=""{customPair.Value.Value}""" };| string filter = $@"contexts.""{custom.Keys.Single()}"":*"; | ||
| var options = new ListObjectsOptions { Filter = filter }; |
There was a problem hiding this comment.
The construction of the filter string using .Single() is fragile. If the test data in the custom dictionary changes, this could break. It's better to extract the key in a more robust way to build the filter string.
var customPair = Assert.Single(custom);
var options = new ListObjectsOptions { Filter = $@"contexts."{customPair.Key}":*" };| string filter = $@"-contexts.""{custom.Keys.Single()}""=""{custom.Values.Single().Value}"""; | ||
| var options = new ListObjectsOptions { Filter = filter }; |
There was a problem hiding this comment.
The construction of the filter string using .Single() is fragile. If the test data in the custom dictionary changes, this could break. It's better to extract the key-value pair in a more robust way to build the filter string.
var customPair = Assert.Single(custom);
var options = new ListObjectsOptions { Filter = $@"-contexts."{customPair.Key}"=""{customPair.Value.Value}""" };| string filter = $@"-contexts.""{custom.Keys.Single()}"":*"; | ||
| var options = new ListObjectsOptions { Filter = filter }; |
There was a problem hiding this comment.
The construction of the filter string using .Single() is fragile. If the test data in the custom dictionary changes, this could break. It's better to extract the key in a more robust way to build the filter string.
var customPair = Assert.Single(custom);
var options = new ListObjectsOptions { Filter = $@"-contexts."{customPair.Key}":*" };|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal(modifiedCustom[modifiedCustom.Keys.Single()].Value, updated.Contexts.Custom[modifiedCustom.Keys.Single()].Value); |
There was a problem hiding this comment.
Using .Single() on Keys and Values collections separately is fragile and less readable. A better approach is to get the single KeyValuePair from the dictionary using Assert.Single and then use its Key and Value properties. This makes the test more robust and the intent clearer.
var modifiedPair = Assert.Single(modifiedCustom);
Assert.Equal(modifiedPair.Value.Value, updated.Contexts.Custom[modifiedPair.Key].Value);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature. The tests cover various scenarios including creating, getting, listing, updating, patching, and copying objects with contexts. The changes are well-structured. I've made a few suggestions to improve the robustness and clarity of some test assertions.
| _fixture.SingleVersionBucket, destination.Name, | ||
| _fixture.MultiVersionBucket, destination.Name); | ||
| Object fetched = _fixture.Client.GetObject(_fixture.MultiVersionBucket, destination.Name); | ||
| Assert.True(result.Contexts.Custom.Keys.SequenceEqual(fetched.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
The assertion using SequenceEqual on dictionary keys is not robust as it relies on the order of keys, which is not guaranteed. Additionally, it only validates the keys and not the values. A more reliable approach is to check for the equality of counts and then verify that all key-value pairs from the source context exist in the fetched context.
Assert.Equal(result.Contexts.Custom.Count, fetched.Contexts.Custom.Count);
Assert.True(result.Contexts.Custom.All(p =>
fetched.Contexts.Custom.TryGetValue(p.Key, out var val) && val.Value == p.Value.Value));| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); | ||
| Assert.True(destination.Contexts.Custom.Keys.SequenceEqual(obj.Contexts.Custom.Keys)); |
There was a problem hiding this comment.
This assertion using SequenceEqual is brittle as it depends on the order of keys from a dictionary, which is not guaranteed. It is also redundant because the assertions on lines 119 (count check) and 121 (key-value pair check) already provide a more robust validation of the Custom contexts. This line can be removed.
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.True(obj.Contexts.Custom.Keys.Single().SequenceEqual(custom.Keys.Single())); |
There was a problem hiding this comment.
While SequenceEqual works for comparing strings, Assert.Equal is more idiomatic and clearly conveys the intent of comparing two strings for equality.
Assert.Equal(custom.Keys.Single(), obj.Contexts.Custom.Keys.Single());| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.True(obj.Contexts.Custom.Keys.Single().SequenceEqual(custom.Keys.Single())); |
There was a problem hiding this comment.
While SequenceEqual works for comparing strings, Assert.Equal is more idiomatic and clearly conveys the intent of comparing two strings for equality.
Assert.Equal(custom.Keys.Single(), obj.Contexts.Custom.Keys.Single());| var result = _fixture.Client.UploadObject(destination, source); | ||
| Assert.Equal(destination.Name, result.Name); | ||
| Assert.Equal(destination.Bucket, result.Bucket); | ||
| Assert.True(destination.Contexts.Custom.Keys.Single().SequenceEqual(result.Contexts.Custom.Keys.Single())); |
There was a problem hiding this comment.
While SequenceEqual works for comparing strings, Assert.Equal is more idiomatic and clearly conveys the intent of comparing two strings for equality.
Assert.Equal(destination.Contexts.Custom.Keys.Single(), result.Contexts.Custom.Keys.Single());
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature. The changes are well-structured and cover various scenarios including upload, get, copy, patch, update, and list operations. My review focuses on improving the robustness and clarity of the test assertions. Several tests could be improved to be more explicit about their expectations (e.g., asserting the exact number of items) and to be more resilient (e.g., by using Assert.Single and deconstruction for cleaner and safer assertions). I've also identified a few tests that were not fully isolated or could pass incorrectly under certain conditions, and I've provided suggestions to address these issues.
| string filter = $@"contexts.""{custom.Keys.Single()}""=""{custom[custom.Keys.Single()].Value}"""; | ||
| var options = new ListObjectsOptions { Filter = filter }; | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(custom.Keys.Single(), obj.Contexts.Custom.Keys.Single()); | ||
| Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value); | ||
| } |
There was a problem hiding this comment.
This part of the test has a few issues:
- It will pass even if no matching objects are found, as the
foreachloop will simply not execute. - The assertions inside the loop are brittle due to repeated calls to
Single(). - The test isn't fully isolated as it lists all objects in the bucket and could be affected by other tests. It's better to filter the results to only the object created in this test.
The suggested change addresses these points by filtering for the specific object and using more robust assertions.
var (key, value) = custom.Single();
string filter = $@"contexts."{key}"="{value.Value}"";
var options = new ListObjectsOptions { Filter = filter };
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).Where(o => o.Name == destination.Name).ToList();
var obj = Assert.Single(objects);
var fetchedContext = Assert.Single(obj.Contexts.Custom);
Assert.Equal(key, fetchedContext.Key);
Assert.Equal(value.Value, fetchedContext.Value.Value);| string filter = $@"contexts.""{custom.Keys.Single()}"":*"; | ||
| var options = new ListObjectsOptions { Filter = filter }; | ||
| var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).ToList(); | ||
| foreach (var obj in objects) | ||
| { | ||
| Assert.Equal(custom.Keys.Single(), obj.Contexts.Custom.Keys.Single()); | ||
| Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value); | ||
|
|
||
| } |
There was a problem hiding this comment.
This part of the test has a few issues:
- It will pass even if no matching objects are found, as the
foreachloop will simply not execute. - The assertions inside the loop are brittle due to repeated calls to
Single(). - The test isn't fully isolated as it lists all objects in the bucket and could be affected by other tests. It's better to filter the results to only the object created in this test.
The suggested change addresses these points by filtering for the specific object and using more robust assertions.
var (key, value) = custom.Single();
string filter = $@"contexts."{key}":*";
var options = new ListObjectsOptions { Filter = filter };
var objects = _fixture.Client.ListObjects(_fixture.ReadBucket, options: options).Where(o => o.Name == destination.Name).ToList();
var obj = Assert.Single(objects);
var fetchedContext = Assert.Single(obj.Contexts.Custom);
Assert.Equal(key, fetchedContext.Key);
Assert.Equal(value.Value, fetchedContext.Value.Value);|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal(2, updated.Contexts.Custom.Keys.Count); |
There was a problem hiding this comment.
This assertion is incomplete. It correctly checks that the number of contexts is 2, but it doesn't verify that both the original and the new contexts are present with their correct values. The test should be expanded to check for the presence and correctness of both contexts.
Assert.Equal(2, updated.Contexts.Custom.Count);
var (newKey, newValue) = modifiedCustom.Single();
Assert.True(updated.Contexts.Custom.TryGetValue(newKey, out var newFetchedValue));
Assert.Equal(newValue.Value, newFetchedValue.Value);
var (oldKey, oldValue) = custom.Single();
Assert.True(updated.Contexts.Custom.TryGetValue(oldKey, out var oldFetchedValue));
Assert.Equal(oldValue.Value, oldFetchedValue.Value);| Assert.True(fetched.Contexts.Custom.ContainsKey(newCustom.Keys.Single())); | ||
| Assert.Equal(newCustom.Values.Single().Value, fetched.Contexts.Custom[newCustom.Keys.Single()].Value); | ||
| Assert.False(fetched.Contexts.Custom.ContainsKey(custom.Keys.Single())); |
There was a problem hiding this comment.
These assertions can be made more robust and readable. Using Assert.Single can verify that there's exactly one context and deconstruct the result for cleaner assertions. This also implicitly verifies that the old context was removed.
var (newKey, newValue) = newCustom.Single();
var fetchedContext = Assert.Single(fetched.Contexts.Custom);
Assert.Equal(newKey, fetchedContext.Key);
Assert.Equal(newValue.Value, fetchedContext.Value.Value);| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); | ||
| Assert.True(destination.Contexts.Custom.All(pair => obj.Contexts.Custom.ContainsKey(pair.Key) && obj.Contexts.Custom[pair.Key].Value == pair.Value.Value)); |
There was a problem hiding this comment.
This assertion is correct, but for better readability and consistency with other tests, consider using Assert.Single to verify the context count and its contents, especially since you know there's only one item in destination.Contexts.Custom.
var (key, value) = Assert.Single(destination.Contexts.Custom);
var fetchedContext = Assert.Single(obj.Contexts.Custom);
Assert.Equal(key, fetchedContext.Key);
Assert.Equal(value.Value, fetchedContext.Value.Value);| Assert.True(obj.Contexts.Custom.ContainsKey(custom.Keys.Single())); | ||
| Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
These assertions can be simplified and made more robust by using Assert.Single to verify the number of contexts and deconstructing the key-value pair for cleaner checks.
var (key, value) = custom.Single();
var fetchedContext = Assert.Single(obj.Contexts.Custom);
Assert.Equal(key, fetchedContext.Key);
Assert.Equal(value.Value, fetchedContext.Value.Value);|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal(modifiedCustom[modifiedCustom.Keys.Single()].Value, updated.Contexts.Custom[modifiedCustom.Keys.Single()].Value); |
There was a problem hiding this comment.
This assertion can be made more robust and readable. It's good practice to also assert that there is still only one context after the modification. Using Assert.Single can help with both.
var (key, value) = modifiedCustom.Single();
var fetchedContext = Assert.Single(updated.Contexts.Custom);
Assert.Equal(key, fetchedContext.Key);
Assert.Equal(value.Value, fetchedContext.Value.Value);| Assert.Equal(destination.Contexts.Custom.Keys.Single(), result.Contexts.Custom.Keys.Single()); | ||
| Assert.Equal(destination.Contexts.Custom.Values.Single().Value, result.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
These assertions can be made more robust and readable by using Assert.Single to verify there's exactly one context and deconstructing the result for cleaner checks.
var (key, value) = Assert.Single(destination.Contexts.Custom);
var resultContext = Assert.Single(result.Contexts.Custom);
Assert.Equal(key, resultContext.Key);
Assert.Equal(value.Value, resultContext.Value.Value);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the new object context feature, covering upload, get, copy, patch, update, and list operations. The changes also include adding a Filter option to ListObjectsOptions to support filtering by context. The new tests are well-structured and cover various scenarios, including using non-ASCII characters. My review focuses on improving the readability and robustness of assertions in several test cases to make them clearer and more maintainable.
| Assert.True(fetched.Contexts.Custom.ContainsKey(newCustom.Keys.Single())); | ||
| Assert.Equal(newCustom.Values.Single().Value, fetched.Contexts.Custom[newCustom.Keys.Single()].Value); |
There was a problem hiding this comment.
These assertions can be made more readable and robust by first extracting the single key-value pair from the newCustom dictionary. This avoids repeated calls to .Single() and separate dictionary lookups.
var newContext = Assert.Single(newCustom);
Assert.True(fetched.Contexts.Custom.ContainsKey(newContext.Key));
Assert.Equal(newContext.Value.Value, fetched.Contexts.Custom[newContext.Key].Value);| Assert.Equal(_fixture.MultiVersionBucket, obj.Bucket); | ||
| Assert.NotNull(obj.Contexts); | ||
| Assert.Equal(obj.Contexts.Custom.Count, result.Contexts.Custom.Count); | ||
| Assert.True(destination.Contexts.Custom.All(pair => obj.Contexts.Custom.ContainsKey(pair.Key) && obj.Contexts.Custom[pair.Key].Value == pair.Value.Value)); |
There was a problem hiding this comment.
This All assertion is correct but can be hard to read and debug. Since you expect a single context, consider using Assert.Single to retrieve both the expected and actual contexts, and then assert on their Key and Value properties separately. This would make the test's intent clearer and more aligned with other assertions in the test suite.
| Assert.True(obj.Contexts.Custom.ContainsKey(custom.Keys.Single())); | ||
| Assert.Equal(custom.Values.Single().Value, obj.Contexts.Custom[custom.Keys.Single()].Value); |
There was a problem hiding this comment.
These assertions can be simplified for better readability and robustness. Instead of checking for the key and then fetching the value in separate steps, you can retrieve the single context from both dictionaries and compare them.
var expectedContext = Assert.Single(custom);
var actualContext = Assert.Single(obj.Contexts.Custom);
Assert.Equal(expectedContext.Key, actualContext.Key);
Assert.Equal(expectedContext.Value.Value, actualContext.Value.Value);|
|
||
| Object obj = new Object { Name = destination.Name, Bucket = _fixture.SingleVersionBucket, ContentType = "text/plain", Contexts = new Object.ContextsData { Custom = modifiedCustom } }; | ||
| var updated = client.PatchObject(obj); | ||
| Assert.Equal(modifiedCustom[modifiedCustom.Keys.Single()].Value, updated.Contexts.Custom[modifiedCustom.Keys.Single()].Value); |
There was a problem hiding this comment.
This assertion is a bit dense and performs multiple dictionary lookups. For better readability and to make the test's intent clearer, consider using Assert.Single to retrieve the modified and updated contexts as KeyValuePairs, and then assert on their properties separately.
| Assert.Equal(destination.Contexts.Custom.Keys.Single(), result.Contexts.Custom.Keys.Single()); | ||
| Assert.Equal(destination.Contexts.Custom.Values.Single().Value, result.Contexts.Custom[custom.Keys.Single()].Value); | ||
| Assert.NotNull(result.Contexts.Custom[custom.Keys.Single()].CreateTimeRaw); | ||
| Assert.NotNull(result.Contexts.Custom[custom.Keys.Single()].UpdateTimeRaw); |
There was a problem hiding this comment.
These assertions are a bit complex and perform multiple dictionary lookups. You can improve readability and robustness by using Assert.Single to get the KeyValuePair for both expected and actual contexts, and then perform assertions on their properties.
var expectedContext = Assert.Single(destination.Contexts.Custom);
var actualContext = Assert.Single(result.Contexts.Custom);
Assert.Equal(expectedContext.Key, actualContext.Key);
Assert.Equal(expectedContext.Value.Value, actualContext.Value.Value);
Assert.NotNull(actualContext.Value.CreateTimeRaw);
Assert.NotNull(actualContext.Value.UpdateTimeRaw);
mahendra-google
changed the title
mahendra-google
requested review from
gurusai-voleti,
shubhangi-google and
thiyaguk09
mahendra-google
force-pushed
the
object-context
branch
from
614c529 to
24f2e5f
Compare
…for object context
mahendra-google
force-pushed
the
object-context
branch
from
39e5d31 to
36af17d
Compare
1 participant
This PR introduces the ability to filter ListObjects results with the introduction of
Filterproperty inListObjectOptionsand adds comprehensive integration tests for theObject Contextfeature. Please see b/434726579