Tests .NET #1221
Tests .NET #1221
Conversation
Replaces switch on JsonTokenType with a recursive method using JsonElement.ValueKind for more robust and accurate type inference. This improves handling of nested objects and arrays, and unifies the logic for converting JSON values to .NET types.
Simplifies and standardizes the deserialization of model properties from dictionaries, removing special handling for JsonElement and streamlining array and primitive type conversions. This improves code readability and maintainability in generated model classes.
Updated the From method in the model template to check for the existence of optional properties in the input map before assigning values. This prevents errors when optional properties are missing from the input dictionary. (for examle in model: User, :-/ )
Improves the From() method in Model.cs.twig to handle nullable and array properties more robustly, using helper macros for parsing arrays and sub-schemas. This change ensures correct handling of optional fields and type conversions, reducing runtime errors and improving code maintainability. Also removes an unnecessary blank line in ServiceTemplate.cs.twig.
Fields with null values in multipart are now omitted (so they don't turn into empty strings).
Introduces a new parse_value Twig function in DotNet.php to centralize and simplify value parsing logic for model properties. Updates Model.cs.twig to use this function, reducing template complexity and improving maintainability.
Remove null-forgiving operator (!) from optional array mappings and use null-safe casting to preserve null vs empty semantics in generated models.
Adds conditional import of the Enums namespace in the Model.cs.twig template only when the model definition contains enum properties. This prevents unnecessary imports and improves template clarity.
Introduces a ToEnumerable extension method to unify array and enumerable conversions in generated .NET code. Updates code generation logic to use ToEnumerable for array properties, simplifying and improving type safety. Also adds necessary using statement for Extensions in generated model files.
Introduces comprehensive test templates for the .NET SDK, including unit tests for client, models, enums, converters, exceptions, and utility classes. Updates the DotNet language generator to support test file generation and adds new Twig filters and functions to facilitate test code creation.
Introduces a 'Run Tests' step in the sdk-build-validation workflow for multiple SDKs. This step runs the appropriate test command for each SDK and handles cases where no tests are available.
WalkthroughAdds a "Run Tests" step to the sdk-build-validation GitHub Actions workflow and introduces extensive .NET test scaffolding (new Tests.csproj, .sln updates, and 20+ test templates under templates/dotnet/Package.Tests). Implements many new test templates (clients, converters, enums, models, services, utilities), new Twig helpers/filters for test value/type generation ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
templates/dotnet/Package/Extensions/Extensions.cs.twig (1)
27-50: Encode query strings per key/value; avoid escaping the whole query.
Current code risks malformed queries and locale-sensitive values.+using System.Globalization; @@ - foreach (var kvp in parameters) + foreach (var kvp in parameters) { switch (kvp.Value) { case null: continue; case IList list: foreach (var item in list) { - query.Add($"{kvp.Key}[]={item}"); + var k = Uri.EscapeDataString($"{kvp.Key}[]"); + var v = Uri.EscapeDataString(Convert.ToString(item, CultureInfo.InvariantCulture) ?? string.Empty); + query.Add($"{k}={v}"); } break; default: - query.Add($"{kvp.Key}={kvp.Value.ToString()}"); + var key = Uri.EscapeDataString(kvp.Key); + var val = Uri.EscapeDataString(Convert.ToString(kvp.Value, CultureInfo.InvariantCulture) ?? string.Empty); + query.Add($"{key}={val}"); break; } } - return Uri.EscapeUriString(string.Join("&", query)); + return string.Join("&", query);templates/dotnet/Package/Client.cs.twig (3)
247-291: Dispose HttpResponseMessage to avoid handler/socket leaks.Responses are never disposed. Wrap SendAsync with a using to prevent resource leaks on long‑running clients.
Apply:
- var response = await _httpForRedirect.SendAsync(request); + using var response = await _httpForRedirect.SendAsync( + request, + HttpCompletionOption.ResponseHeadersRead);
302-375: Dispose response; optional: support text responses.
- Same disposal issue as Redirect(); wrap in using.
- Optional: for non‑JSON text content, returning byte[] may be surprising. Consider returning string when typeof(T) == typeof(string) and content-type starts with text/.
- var response = await _http.SendAsync(request); + using var response = await _http.SendAsync(request); @@ - else - { - return ((await response.Content.ReadAsByteArrayAsync()) as T)!; - } + else + { + // Optional: text bodies + if (typeof(T) == typeof(string)) + { + object s = await response.Content.ReadAsStringAsync(); + return (T)s; + } + var bytes = await response.Content.ReadAsByteArrayAsync(); + return (T)(object)bytes; + }
377-542: ChunkedUpload has correctness bugs: off‑by‑one, short reads ignored, wrong offset/progress, and potential stream leak.
- Off‑by‑one: bytes path uses ChunkSize - 1, dropping a byte per chunk.
- Short reads: assumes a single ReadAsync fills the buffer; not guaranteed. Must respect bytesRead and slice ByteArrayContent.
- Offset increment uses ChunkSize, causing gaps/duplication when bytesRead < ChunkSize.
- Progress uses integer division; always 0 until completion.
- Path source stream never disposed.
Fix minimally:
@@ - var size = 0L; - switch(input.SourceType) + var size = 0L; + Stream? stream = null; + bool disposeStream = false; + byte[]? sourceBytes = null; + switch(input.SourceType) { case "path": - var info = new FileInfo(input.Path); - input.Data = info.OpenRead(); - size = info.Length; + var info = new FileInfo(input.Path); + stream = info.OpenRead(); + disposeStream = true; + size = info.Length; break; case "stream": - var stream = input.Data as Stream; - if (stream == null) + stream = input.Data as Stream; + if (stream == null) throw new InvalidOperationException("Stream data is null"); - size = stream.Length; + if (!stream.CanSeek) + throw new InvalidOperationException("Stream must be seekable"); + size = stream.Length; break; case "bytes": - var bytes = input.Data as byte[]; - if (bytes == null) + sourceBytes = input.Data as byte[]; + if (sourceBytes == null) throw new InvalidOperationException("Byte array data is null"); - size = bytes.Length; + size = sourceBytes.Length; break; }; @@ - var buffer = new byte[Math.Min(size, ChunkSize)]; + var buffer = new byte[Math.Min(size, ChunkSize)]; var result = new Dictionary<string, object?>(); @@ - switch(input.SourceType) + switch(input.SourceType) { case "path": case "stream": - var dataStream = input.Data as Stream; - if (dataStream == null) + var dataStream = stream; + if (dataStream == null) throw new InvalidOperationException("Stream data is null"); - await dataStream.ReadAsync(buffer, 0, (int)size); + var remaining = (int)size; + var readTotal = 0; + while (readTotal < remaining) + { + var n = await dataStream.ReadAsync(buffer, readTotal, remaining - readTotal); + if (n == 0) break; + readTotal += n; + } + if (readTotal <= 0) throw new InvalidOperationException("Failed to read data"); break; case "bytes": - var dataBytes = input.Data as byte[]; - if (dataBytes == null) + var dataBytes = sourceBytes; + if (dataBytes == null) throw new InvalidOperationException("Byte array data is null"); - buffer = dataBytes; + buffer = dataBytes; break; } @@ - var content = new MultipartFormDataContent { - { new ByteArrayContent(buffer), paramName, input.Filename } - }; + var content = new MultipartFormDataContent(); + var part = (input.SourceType == "bytes") + ? new ByteArrayContent(buffer) + : new ByteArrayContent(buffer, 0, (int)size); + content.Add(part, paramName, input.Filename); @@ - while (offset < size) + try + { + while (offset < size) { switch(input.SourceType) { case "path": case "stream": - var stream = input.Data as Stream; - if (stream == null) + if (stream == null) throw new InvalidOperationException("Stream data is null"); - stream.Seek(offset, SeekOrigin.Begin); - await stream.ReadAsync(buffer, 0, ChunkSize); + stream.Seek(offset, SeekOrigin.Begin); + var toRead = (int)Math.Min(size - offset, ChunkSize); + if (buffer.Length < toRead) buffer = new byte[toRead]; + var readTotal = 0; + while (readTotal < toRead) + { + var n = await stream.ReadAsync(buffer, readTotal, toRead - readTotal); + if (n == 0) break; + readTotal += n; + } + if (readTotal <= 0) break; break; case "bytes": - buffer = ((byte[])input.Data) - .Skip((int)offset) - .Take((int)Math.Min(size - offset, ChunkSize - 1)) - .ToArray(); + var len = (int)Math.Min(size - offset, ChunkSize); + if (buffer.Length != len) buffer = new byte[len]; + Array.Copy(sourceBytes!, (int)offset, buffer, 0, len); break; } - var content = new MultipartFormDataContent { - { new ByteArrayContent(buffer), paramName, input.Filename } - }; + var content = new MultipartFormDataContent(); + var contentLength = (int)Math.Min(size - offset, ChunkSize); + var part = new ByteArrayContent(buffer, 0, contentLength); + content.Add(part, paramName, input.Filename); @@ - headers["Content-Range"] = - $"bytes {offset}-{Math.Min(offset + ChunkSize - 1, size - 1)}/{size}"; + var end = offset + contentLength - 1; + headers["Content-Range"] = $"bytes {offset}-{end}/{size}"; @@ - offset += ChunkSize; + offset += contentLength; @@ - onProgress?.Invoke( + onProgress?.Invoke( new UploadProgress( id: id, - progress: Math.Min(offset, size) / size * 100, + progress: (double)Math.Min(offset, size) / size * 100d, sizeUploaded: Math.Min(offset, size), chunksTotal: chunksTotal, chunksUploaded: chunksUploaded)); } + } + finally + { + if (disposeStream) + stream?.Dispose(); + }This keeps behavior but fixes correctness and leaks.
templates/dotnet/Package/Models/UploadProgress.cs.twig (1)
1-2: Place UploadProgress under Appwrite.Models for consistencyTo match result types and other templates that reference Appwrite.Models, move this class into the Models namespace.
Apply:
-namespace {{ spec.title | caseUcfirst }} +namespace {{ spec.title | caseUcfirst }}.Models {src/SDK/Language/DotNet.php (1)
162-186: Qualify InputFile with Models namespace to ensure compilation in services without definitionsThe review is correct. InputFile is declared in the
Appwrite.Modelsnamespace (confirmed in template), yet getTypeName returns an unqualifiedInputFileat line 179. This causes compilation failures when a service method uses InputFile but has no model definitions, because:
- ServiceTemplate.cs.twig only generates
using Appwrite.Models;conditionally (ifspec.definitions is not empty)- Without that using statement, unqualified
InputFileis undefined- Other models use the qualified pattern at line 590:
'Appwrite.Models.' . $resultRequired fix:
- self::TYPE_FILE => 'InputFile', + self::TYPE_FILE => 'Appwrite.Models.InputFile',This aligns InputFile with the enum and model qualification convention, ensuring generated code compiles regardless of whether definitions exist.
templates/dotnet/Package/Models/Model.cs.twig (3)
42-56: From(map): safer cast for “data” and alignment with optional inputs.
- data cast can throw if map["data"] isn’t exactly Dictionary<string, object>. Prefer as + coalesce.
- Named args already use removeDollarSign; good.
Apply this change to the data mapping:
- data: map.TryGetValue("data", out var dataValue) ? (Dictionary<string, object>)dataValue : map + data: map.TryGetValue("data", out var dataValue) ? (dataValue as Dictionary<string, object>) ?? map : mapIf any non-required property remains non-nullable, From(...) can still pass null; see prior comment.
58-67: ToMap: potential NullReference on optional nested models/arrays.For sub-schema properties, ToMap() is called unguarded; optional properties can be null. Use null-conditional:
- { "{{ property.name }}", {% if property.sub_schema %}{% if property.type == 'array' %}{{ property_name(definition, property) | overrideProperty(definition.name) }}.Select(it => it.ToMap()){% else %}{{ property_name(definition, property) | overrideProperty(definition.name) }}.ToMap(){% endif %}{% elseif property.enum %}{{ property_name(definition, property) | overrideProperty(definition.name) }}{% if not property.required %}?{% endif %}.Value{% else %}{{ property_name(definition, property) | overrideProperty(definition.name) }}{% endif %}{{ ' }' }}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %} + { "{{ property.name }}", {% if property.sub_schema %}{% if property.type == 'array' %}{{ property_name(definition, property) | overrideProperty(definition.name) }}{% if not property.required %}?{% endif %}.Select(it => it.ToMap()){% else %}{{ property_name(definition, property) | overrideProperty(definition.name) }}{% if not property.required %}?{% endif %}.ToMap(){% endif %}{% elseif property.enum %}{{ property_name(definition, property) | overrideProperty(definition.name) }}{% if not property.required %}?{% endif %}.Value{% else %}{{ property_name(definition, property) | overrideProperty(definition.name) }}{% endif %}{{ ' }' }}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %}Prevents NRE when optional nested objects/arrays are absent.
76-82: Duplicate method signature and invalid return/cast in ConvertTo overload confirmed—compile errors require immediate fix.The verification confirms all critical issues:
- Duplicate signatures (lines 70 and 78): Identical method
public T ConvertTo<T>(Func<Dictionary<string, object>, T> fromJson)appears twice and will compile-fail when both conditions are true.- Invalid cast:
(T){{ ... }}.Select(...)castsIEnumerable<T>toT, which is a type error.- Inconsistent naming: Line 79 uses
caseUcfirstwhile the template consistently usesproperty_name(definition, property) | overrideProperty(definition.name)elsewhere (lines 18, 35, 61).Rename and fix the method to return
IEnumerable<T>:- public T ConvertTo<T>(Func<Dictionary<string, object>, T> fromJson) => - (T){{ property.name | caseUcfirst | escapeKeyword }}.Select(it => it.ConvertTo(fromJson)); + public IEnumerable<T> Convert{{ property_name(definition, property) | overrideProperty(definition.name) }}To<T>(Func<Dictionary<string, object>, T> fromJson) => + {{ property_name(definition, property) | overrideProperty(definition.name) }}?.Select(it => it.ConvertTo(fromJson)) ?? Enumerable.Empty<T>();This resolves the signature collision, fixes the type mismatch, uses consistent naming, and handles nulls safely.
🧹 Nitpick comments (18)
templates/dotnet/Package.Tests/IDTests.cs.twig (1)
8-56: Consider additional edge case tests.The current test coverage is solid for common scenarios. For more comprehensive coverage, consider adding tests for:
ID.Custom(null)- to verify null handling behaviorID.Unique(-1)orID.Unique(0)- to verify padding boundary conditionsID.Unique(1000)- to verify behavior with very large padding valuestemplates/dotnet/Package.Tests/Models/InputFileTests.cs.twig (2)
77-108: Consider resource disposal pattern for streams.The MemoryStream instances created in these tests are not explicitly disposed. While the memory impact is negligible in tests and disposing might interfere with verifying the stream is stored correctly, consider adding a comment explaining why disposal is omitted, or restructure tests to validate stream storage and then dispose.
1-217: Consider adding error case tests.The test suite provides excellent coverage of happy paths and edge cases. Consider adding tests for error scenarios to document expected behavior:
FromPathwith null or empty pathFromStreamwith null streamFromByteswith null byte arrayFromFileInfowith null FileInfoThis would clarify whether these methods should throw exceptions or handle nulls gracefully.
templates/dotnet/Package.Tests/.gitignore (1)
1-23: Consider adding a few common ignores.
Optional quality-of-life entries: .DS_Store, .vscode/, BenchmarkDotNet.Artifacts/, coverage.cobertura.xml.Apply this diff:
+# OS/editor +.DS_Store +.vscode/ + +# Benchmarks +BenchmarkDotNet.Artifacts/ + +# Other coverage formats +coverage.cobertura.xmltemplates/dotnet/Package/Exception.cs.twig (1)
22-25: Make inner exception nullable to match BCL and avoid warnings.
System.Exception ctor accepts Exception?; this keeps nullable flow clean.- public {{spec.title | caseUcfirst}}Exception(string message, Exception inner) + public {{spec.title | caseUcfirst}}Exception(string message, Exception? inner) : base(message, inner) { }templates/dotnet/Package.Tests/Tests.csproj.twig (1)
3-8: Enable nullable reference types in tests.
Catches nullability issues early and aligns with SDK.<PropertyGroup> <TargetFramework>net8.0</TargetFramework> <RootNamespace>{{ spec.title | caseUcfirst }}.Tests</RootNamespace> <IsPackable>false</IsPackable> <IsTestProject>true</IsTestProject> + <Nullable>enable</Nullable> </PropertyGroup>templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig (1)
10-16: Recursive conversion rewrite: solid improvement; minor enhancements optional.The new Read + ConvertElement flow is clearer and handles nulls properly. Consider (optional) also:
- Prefer DateTimeOffset via TryGetDateTimeOffset for timezone fidelity before falling back to DateTime.
- For large/precise numbers, attempt decimal when representable to avoid double precision loss (guarded by try/catch to stay compatible).
Also applies to: 18-64
templates/dotnet/Package.Tests/Models/ModelTests.cs.twig (2)
300-306: Check all model properties for read‑only, not just the first.Iterate all properties to catch accidental public setters.
- {%~ for property in definition.properties | slice(0, 1) %} + {%~ for property in definition.properties %} var propertyInfo = typeof({{ spec.title | caseUcfirst }}.Models.{{ DefinitionClass }}).GetProperty("{{ property_name(definition, property) | overrideProperty(definition.name) }}"); Assert.NotNull(propertyInfo); Assert.Null(propertyInfo.GetSetMethod()); {%~ endfor %}
1-16: Minor: macro emits only required sub‑schema props.That’s fine for a smoke test. If From() requires optional fields with defaults, consider extending the macro to include a representative optional as well. No action required now.
src/SDK/Language/DotNet.php (5)
545-551: escapeCsString should cover control characters, not only quotes and backslashesEscaping just
\"and\can leak newlines, tabs, etc., into string literals in generated tests.Apply:
- new TwigFilter('escapeCsString', function ($value) { - if (is_string($value)) { - return addcslashes($value, '\\"'); - } - return $value; - }), + new TwigFilter('escapeCsString', function ($value) { + if (is_string($value)) { + // Escape backslash, quote, and common control chars + return addcslashes($value, "\\\"\n\r\t\f\v"); + } + return $value; + }),
616-643: Use toPascalCase instead of ucfirst for schema/enum class namesucfirst won’t handle delimiters; toPascalCase already exists and is used elsewhere.
- $subSchema = \ucfirst($property['sub_schema']); + $subSchema = $this->toPascalCase($property['sub_schema']); ... - $enumClass = \ucfirst($enumName); + $enumClass = $this->toPascalCase($enumName);
645-659: Safer conversions in array parsingDirect casts
(bool)xcan throw for “1”/“true”/0/1. Prefer Convert.* for primitives to match non‑strict JSON inputs.- $selectExpression = match ($itemsType) { - 'string' => 'x.ToString()', - 'integer' => 'Convert.ToInt64(x)', - 'number' => 'Convert.ToDouble(x)', - 'boolean' => '(bool)x', - default => 'x' - }; + $selectExpression = match ($itemsType) { + 'string' => 'x?.ToString()', + 'integer' => 'Convert.ToInt64(x)', + 'number' => 'Convert.ToDouble(x)', + 'boolean' => 'Convert.ToBoolean(x)', + default => 'x' + };
671-679: Boolean parsing for scalars: avoid direct castsUse Convert.ToBoolean for required and nullable branches to accept 0/1/"true"/"false".
- if ($required) { - return "({$typeName}){$mapAccess}"; - } - return "({$typeName}?){$v}"; + if ($required) { + return "Convert.ToBoolean({$mapAccess})"; + } + return "{$v} == null ? (bool?)null : Convert.ToBoolean({$v})";
693-709: Anonymous object generation may emit invalid identifiers for JSON keysformatCSharpAnonymousObject emits
{ key-name = ... }which won’t compile for keys with dashes/spaces. Prefer a Dictionary initializer for arbitrary keys.Consider emitting:
new Dictionary<string, object> { ["key-name"] = ..., ["another key"] = ... }Alternatively, sanitize keys into valid identifiers and document the transformation.
Also applies to: 714-732
templates/dotnet/Package.Tests/Services/ServiceTests.cs.twig (3)
186-198: Actually verify parameter passing instead of Assert.True(true).The “WithParameters_PassesCorrectParameters” test doesn’t validate what was sent. Verify that the payload dictionary contains the expected keys/values (at least for the first few required parameters you materialize).
- // Assert - parameters were set correctly (implicitly tested by successful call) - Assert.True(true); + // Assert - parameters were set correctly on the client call + _mockClient.Verify(c => c.Call<{{ utils.resultType(spec.title, method) }}>( + It.IsAny<string>(), + It.IsAny<string>(), + It.IsAny<Dictionary<string, string>>(), + It.Is<Dictionary<string, object>>(m => + { + var ok = true; + {%~ for parameter in method.parameters.all | filter((param) => param.required) | slice(0, 3) ~%} + ok = ok && m.TryGetValue("{{parameter.name}}", out var v{{loop.index0}}) && Equals(v{{loop.index0}}, {{parameter.name | caseCamel | escapeKeyword}}); + {%~ endfor ~%} + return ok; + }) + {% if method.responseModel %}, It.IsAny<Func<Dictionary<string, object>, {{ utils.resultType(spec.title, method) }}>>() {% else %}, null {% endif %} + ), Times.Once);
124-150: Also verify arguments for Redirect and ChunkedUpload flows.For webAuth (Redirect) and multipart (ChunkedUpload), you only check Times.Once. Add Verify with expected HTTP method/path and non-empty params to catch regressions.
Also applies to: 131-141
9-9: Scope the obsolete warning suppression.#pragma warning disable CS0618 at file scope hides unrelated obsoletions. Narrow it to the minimal region or remove if no longer needed.
templates/dotnet/Package.Tests/Converters/ObjectToInferredTypesConverterTests.cs.twig (1)
174-185: Verify the actual DateTime value.The test only checks that the result is of type
DateTime, but doesn't verify that the date/time was parsed correctly. This leaves a gap in test coverage where parsing could fail silently as long as it returns some DateTime value.Apply this diff to verify the parsed value:
[Fact] public void Read_WithDateTime_ReturnsDateTime() { // Arrange var json = "\"2023-10-16T12:00:00Z\""; // Act var result = JsonSerializer.Deserialize<object>(json, _options); // Assert Assert.IsType<DateTime>(result); + var dateTime = (DateTime)result; + Assert.Equal(new DateTime(2023, 10, 16, 12, 0, 0, DateTimeKind.Utc), dateTime); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/sdk-build-validation.yml(1 hunks)src/SDK/Language/DotNet.php(4 hunks)templates/dotnet/Package.Tests/.gitignore(1 hunks)templates/dotnet/Package.Tests/ClientTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Converters/ObjectToInferredTypesConverterTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Converters/ValueClassConverterTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Enums/EnumTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/ExceptionTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/IDTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Models/InputFileTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Models/ModelTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/PermissionTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/QueryTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/RoleTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Services/ServiceTests.cs.twig(1 hunks)templates/dotnet/Package.Tests/Tests.csproj.twig(1 hunks)templates/dotnet/Package.Tests/UploadProgressTests.cs.twig(1 hunks)templates/dotnet/Package.sln(1 hunks)templates/dotnet/Package/Client.cs.twig(6 hunks)templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig(1 hunks)templates/dotnet/Package/Exception.cs.twig(1 hunks)templates/dotnet/Package/Extensions/Extensions.cs.twig(2 hunks)templates/dotnet/Package/Models/InputFile.cs.twig(2 hunks)templates/dotnet/Package/Models/Model.cs.twig(2 hunks)templates/dotnet/Package/Models/UploadProgress.cs.twig(1 hunks)templates/dotnet/Package/Query.cs.twig(1 hunks)templates/dotnet/Package/Role.cs.twig(2 hunks)templates/dotnet/Package/Services/ServiceTemplate.cs.twig(0 hunks)templates/dotnet/base/utils.twig(1 hunks)
💤 Files with no reviewable changes (1)
- templates/dotnet/Package/Services/ServiceTemplate.cs.twig
🧰 Additional context used
🧬 Code graph analysis (1)
src/SDK/Language/DotNet.php (1)
src/SDK/Language.php (1)
toPascalCase(96-99)
🔇 Additional comments (26)
templates/dotnet/Package.Tests/IDTests.cs.twig (2)
1-58: Solid test coverage for ID utility.The test suite provides good coverage of the ID utility class, including unique ID generation, custom padding, uniqueness verification, and custom string handling with various inputs (empty strings, special characters). The structure properly follows Xunit conventions.
14-14: Verify that ID length is part of the API contract.The tests assert specific lengths (20 for default, 13 + padding for custom padding). If these lengths are implementation details rather than guaranteed API behavior, the tests may break unnecessarily when the ID generation algorithm evolves.
Confirm that these specific length requirements are part of the public API contract. If they're not guaranteed, consider testing for minimum lengths or non-empty results instead.
Also applies to: 24-24
templates/dotnet/Package/Models/InputFile.cs.twig (1)
2-2: LGTM! Namespace parameterization improves template reusability.The change from hardcoded
Appwrite.Extensionsto dynamic{{ spec.title | caseUcfirst }}.Extensionsaligns with the namespace pattern used throughout the template and makes it properly reusable for different SDK projects.templates/dotnet/Package.Tests/Models/InputFileTests.cs.twig (3)
53-75: Excellent resource cleanup pattern.The test properly uses a try-finally block to ensure temp file cleanup, which is good practice even in test code.
110-158: Excellent edge case coverage.The FromBytes tests include good edge case coverage with empty byte arrays and image data scenarios.
202-215: Good defensive testing of default initialization.Validating that the default constructor initializes all properties to non-null values helps prevent NullReferenceExceptions in consuming code.
templates/dotnet/Package/Query.cs.twig (2)
277-277: Formatting-only change — OK.
No behavioral impact.
209-215: Harden Or/And against invalid JSON (avoid nulls).
JsonSerializer.Deserialize can return null; you’d end up serializing null entries. Throw early or filter.
[ suggest_recommended_refactor ]
Apply this diff:- public static string Or(List<string> queries) { - return new Query("or", null, queries.Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions)).ToList()).ToString(); - } + public static string Or(List<string> queries) { + var list = queries + .Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions) + ?? throw new JsonException("Invalid query JSON in Or")) + .ToList(); + return new Query("or", null, list).ToString(); + } - public static string And(List<string> queries) { - return new Query("and", null, queries.Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions)).ToList()).ToString(); - } + public static string And(List<string> queries) { + var list = queries + .Select(q => JsonSerializer.Deserialize<Query>(q, Client.DeserializerOptions) + ?? throw new JsonException("Invalid query JSON in And")) + .ToList(); + return new Query("and", null, list).ToString(); + }Also applies to: 213-215
templates/dotnet/Package.Tests/Tests.csproj.twig (1)
11-22: Update outdated NuGet packages before next release.Several packages in the test project are outdated:
- Microsoft.NET.Test.Sdk: 17.11.1 → 18.0.0 (major version)
- xunit: 2.9.2 → 2.9.3 (patch)
- xunit.runner.visualstudio: 2.8.2 → 3.1.5 (major version)
- coverlet.collector: 6.0.2 → 6.0.4 (patch)
- Moq: 4.20.72 (current)
Verify compatibility before updating, particularly for the major version bumps. Test the project thoroughly after upgrades.
templates/dotnet/Package/Client.cs.twig (3)
91-110: Protected parameterless ctor: good for mocking; confirm header parity.Looks good. One caveat: it omits spec.global.defaultHeaders that the primary ctor injects. If tests or consumers rely on those defaults, proxies created via this ctor may behave differently. Either mirror the same Twig block here or confirm tests set headers explicitly before use.
176-182: Null-check in multipart: LGTM.Skipping null parameter values avoids spurious empty fields in multipart bodies.
293-300: Virtual overload: LGTM.Keeping the non‑generic shim and marking it virtual is fine for mocking.
templates/dotnet/Package.Tests/Models/ModelTests.cs.twig (1)
17-23: Namespace/tests scaffold: good coverage shape.The test set exercises construction, maps, round‑trip, convert‑to, and immutability. Once the namespace fix lands, this template looks solid.
templates/dotnet/Package/Role.cs.twig (1)
1-1: Namespace templating: LGTM.Aligns with the rest of the templated code and the new tests’ namespaces.
templates/dotnet/Package.Tests/PermissionTests.cs.twig (1)
1-77: Solid, focused coverage of permission format helpersAssertions cover default and parameterized roles across all verbs. Looks good.
templates/dotnet/Package.Tests/UploadProgressTests.cs.twig (1)
8-28: Solid coverage for UploadProgress getters and invariants.Also applies to: 30-40, 42-52, 54-64, 66-76, 78-88, 90-100, 102-112, 114-122, 124-135, 137-145, 147-164
templates/dotnet/Package.Tests/ExceptionTests.cs.twig (1)
66-78: Constructor overload confirmed—no changes needed.The verification confirms that the
{{spec.title | caseUcfirst}}Exception(string message, Exception inner)constructor exists at lines 22-25 ofException.cs.twigand correctly delegates to the base class viabase(message, inner). The test properly exercises this overload as expected.templates/dotnet/Package.Tests/QueryTests.cs.twig (1)
13-22: JSON property mapping is correctly configured—no changes needed.Verification confirms that the Query class in
templates/dotnet/Package/Query.cs.twighas explicit[JsonPropertyName]decorators for all three properties (method, attribute, values) that match the lowercase keys used during JSON deserialization in QueryTests.cs.twig. Deserialization will work as expected.templates/dotnet/Package.Tests/Services/ServiceTests.cs.twig (1)
35-37: No changes needed—Moq requirements are satisfied.Verification confirms that all three Client methods are virtual:
Call(line 293),ChunkedUpload(line 377), andRedirect(line 247). The Client class is also not sealed (line 17). The code is already properly configured for Moq mocking.templates/dotnet/Package.Tests/ClientTests.cs.twig (1)
12-35: LGTM - Well-structured client tests.The remaining test methods provide good coverage of the Client class functionality with appropriate assertions:
- Constructor variants with proper verification
- Fluent API methods with state verification
- Property accessors with type checks
- Serialization options validation
- Method chaining behavior
Also applies to: 60-73, 91-115, 145-214
templates/dotnet/Package.Tests/RoleTests.cs.twig (1)
1-108: LGTM - Comprehensive role string formatting tests.The test suite provides excellent coverage of all Role factory methods with clear, focused assertions. Each test verifies the exact string format produced by the role builders, which is critical for ensuring correct permission strings in the SDK.
The test structure is consistent and easy to maintain, with descriptive test names that clearly indicate what's being tested.
templates/dotnet/Package.Tests/Converters/ObjectToInferredTypesConverterTests.cs.twig (1)
1-18: LGTM - Comprehensive converter tests.The test suite provides excellent coverage of the
ObjectToInferredTypesConverterfunctionality:
- Read tests: Verify correct type inference for all JSON value types (primitives, objects, arrays, nested structures, edge cases)
- Write tests: Confirm proper serialization of various .NET types back to JSON
- Round-trip test: Validates data preservation through serialize/deserialize cycles
The test structure is well-organized with clear AAA (Arrange-Act-Assert) pattern, and assertions properly verify both types and values.
Also applies to: 19-173, 188-215, 217-287, 289-311
templates/dotnet/Package/Models/Model.cs.twig (4)
1-1: Macro-driven class naming looks good.Using DefinitionClass with overrideIdentifier reduces reserved-name collisions in generated code.
7-9: Conditional Enums using is correct.Keeps imports minimal and resolves enum type references only when needed.
14-14: Class identifier switch to DefinitionClass is aligned with the macro approach.No issues.
18-18: Add removeDollarSign filter and = default parameters to constructor; ensure nullability encoding for non-required properties.The review comment identifies three valid issues in
templates/dotnet/Package/Models/Model.cs.twig:
Line 27 (constructor parameters): Missing
removeDollarSignfilter, creating inconsistency with theFrom()method (line 46) which already applies it. Dollar signs in property names like$idwould appear as literal$idin named arguments instead of being escaped.Line 27: Non-required properties lack
= defaultparameter defaults, forcing callers to provide explicit values or resort to positional arguments. This reduces ergonomics compared to other language templates (e.g., Swift adds?and makes nulls explicit).Nullability encoding: The
sub_schema(property)macro should return nullable types (e.g.,string?/int?) for non-required properties to prevent null-reference exceptions at runtime or compilation failures for value types.Apply the suggested diff and verify the macro handles C# null-safety for non-required properties:
- {{ sub_schema(property) }} {{ property.name | caseCamel | escapeKeyword }}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %} + {{ sub_schema(property) }} {{ property.name | caseCamel | escapeKeyword | removeDollarSign }}{% if not property.required %} = default{% endif %}{% if not loop.last or (loop.last and definition.additionalProperties) %},{% endif %}Confirm that
sub_schema(property)emits nullable forms (string?,int?, etc.) whenproperty.requiredis false, matching the pattern used in Swift and other language implementations.Likely an incorrect or invalid review comment.
| - name: Run Tests | ||
| working-directory: examples/${{ matrix.sdk }} | ||
| run: | | ||
| case "${{ matrix.sdk }}" in | ||
| web|node|cli|react-native) | ||
| npm test || echo "No tests available" | ||
| ;; | ||
| flutter) | ||
| flutter test || echo "No tests available" | ||
| ;; | ||
| apple|swift) | ||
| swift test || echo "No tests available" | ||
| ;; | ||
| android) | ||
| ./gradlew test || echo "No tests available" | ||
| ;; | ||
| kotlin) | ||
| ./gradlew test || echo "No tests available" | ||
| ;; | ||
| php) | ||
| vendor/bin/phpunit || echo "No tests available" | ||
| ;; | ||
| python) | ||
| python -m pytest || echo "No tests available" | ||
| ;; | ||
| ruby) | ||
| bundle exec rake test || bundle exec rspec || echo "No tests available" | ||
| ;; | ||
| dart) | ||
| dart test || echo "No tests available" | ||
| ;; | ||
| go) | ||
| go test ./... || echo "No tests available" | ||
| ;; | ||
| dotnet) | ||
| dotnet test || echo "No tests available" | ||
| ;; | ||
| *) | ||
| echo "No tests for SDK: ${{ matrix.sdk }}" | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Tests are being swallowed: || echo "No tests available" masks failures
Any failing test will still return success, defeating CI. Detect presence of tests first, then run without masking. Example fix:
- - name: Run Tests
+ - name: Run Tests
working-directory: examples/${{ matrix.sdk }}
run: |
- case "${{ matrix.sdk }}" in
- web|node|cli|react-native)
- npm test || echo "No tests available"
- ;;
+ set -e
+ case "${{ matrix.sdk }}" in
+ web|node|cli|react-native)
+ if node -p "require('./package.json').scripts && require('./package.json').scripts.test ? 1 : 0" >/dev/null 2>&1; then
+ npm test --silent
+ else
+ echo "No tests available"
+ fi
+ ;;
flutter)
- flutter test || echo "No tests available"
+ if [ -d "test" ]; then flutter test; else echo "No tests available"; fi
;;
apple|swift)
- swift test || echo "No tests available"
+ if ls -1 *.package.swift *.swiftpm >/dev/null 2>&1 || [ -d "Tests" ]; then swift test; else echo "No tests available"; fi
;;
android)
- ./gradlew test || echo "No tests available"
+ if [ -f "gradlew" ] && rg -n "testImplementation|androidTestImplementation" -g '!**/build/**' -S . >/dev/null 2>&1; then ./gradlew test; else echo "No tests available"; fi
;;
kotlin)
- ./gradlew test || echo "No tests available"
+ if [ -f "gradlew" ] && rg -n "testImplementation" -g '!**/build/**' -S . >/dev/null 2>&1; then ./gradlew test; else echo "No tests available"; fi
;;
php)
- vendor/bin/phpunit || echo "No tests available"
+ if [ -x "vendor/bin/phpunit" ] || [ -f "phpunit.xml" ] || [ -f "phpunit.xml.dist" ]; then vendor/bin/phpunit; else echo "No tests available"; fi
;;
python)
- python -m pytest || echo "No tests available"
+ if python - <<'PY' 2>/dev/null; then print(); PY
+ then
+ echo "No Python"; exit 0
+ fi
+ if [ -d "tests" ] || rg -n "^pytest" -S pyproject.toml setup.cfg tox.ini >/dev/null 2>&1; then python -m pytest; else echo "No tests available"; fi
;;
ruby)
- bundle exec rake test || bundle exec rspec || echo "No tests available"
+ if [ -f "Rakefile" ]; then bundle exec rake test || bundle exec rspec; elif [ -d "spec" ]; then bundle exec rspec; else echo "No tests available"; fi
;;
dart)
- dart test || echo "No tests available"
+ if [ -d "test" ]; then dart test; else echo "No tests available"; fi
;;
go)
- go test ./... || echo "No tests available"
+ if rg -n "func Test" -t go >/dev/null 2>&1; then go test ./...; else echo "No tests available"; fi
;;
dotnet)
- dotnet test || echo "No tests available"
+ if ls -1 *.sln */*Tests.csproj >/dev/null 2>&1; then dotnet test --nologo --verbosity minimal; else echo "No tests available"; fi
;;
*)
echo "No tests for SDK: ${{ matrix.sdk }}"
;;
esacCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/sdk-build-validation.yml lines 201-241: the current run step
appends `|| echo "No tests available"` which masks test failures; change each
case to first detect whether tests exist or the test command is available (for
node/web/react-native/cli check for a test script in package.json or presence of
test files, for flutter/dart run a lightweight discovery or check for test
directory/files, for swift/apple check for Package.swift tests, for
android/kotlin check for gradle tasks or test sources, for php check for phpunit
config or tests dir, for python check for pytest discovery or tests/ dir, for
ruby check for spec/test dirs, for go check for *_test.go files, for dotnet
check for test project files), and only run the test command without `|| echo
...` so failures surface; if no tests are detected, print the "No tests
available" message and exit 0. Ensure you remove the `|| echo "No tests
available"` from the actual test command invocations so failing tests return
non-zero.
Update InputFileTests to account for platform-specific filename handling by checking the OS and adjusting the expected filename accordingly.
Replaced hardcoded 'Appwrite' namespace references with dynamic '{{ spec.title | caseUcfirst }}' in model and service test templates.
2 participants
What does this PR do?
based on #1138
This PR adds comprehensive testing infrastructure for the .NET SDK generator, including test templates and necessary utilities.
Also added 'Run Tests' step to CI workflow in sdk-build-validation.yml
Added Files and Test Types:
Core Infrastructure:
Tests.csproj.twig- .NET test project template.gitignore- Build artifacts exclusion for testsSDK Common Tests:
ClientTests.cs.twig- Client tests (constructors, configuration, headers, serialization)IDTests.cs.twig- ID generation testsPermissionTests.cs.twig- Permission testsRoleTests.cs.twig- Role testsQueryTests.cs.twig- Query testsExceptionTests.cs.twig- Exception testsUploadProgressTests.cs.twig- Upload progress testsModel and Type Tests:
InputFileTests.cs.twig- Input file testsModelTests.cs.twig- Model tests (constructors, serialization, deserialization)EnumTests.cs.twig- Enum testsConverter Tests:
ObjectToInferredTypesConverterTests.cs.twig- JSON type converter testsValueClassConverterTests.cs.twig- Value class converter testsService Tests:
ServiceTests.cs.twig- API service tests (method calls, parameters, mocking)New Utilities:
escapeCsString- String escaping for C#test_item_type- Array element type determination for testsCode Changes:
src/SDK/Language/DotNet.php- Added test file generation templates and utility functionsThis PR generates tests for each model definition, enum, and service in the API specification, providing comprehensive test coverage for the generated .NET SDK.
Test Plan
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit