Conversation
|
|
||
| public void SeedData() | ||
| { | ||
| DotNetEnv.Env.TraversePath().Load(); |
There was a problem hiding this comment.
I'm not a fan of using third-party tools, like DotNetEnv. I mean, it's great, but it's also one more dependency to keep tabs on. IIRC, in the existing test solution, we just read the file?
| // :uncomment-end: | ||
| // :snippet-end: | ||
| var client = new MongoClient(uri); | ||
| _aggDB = client.GetDatabase("agg_tutorials_db"); |
There was a problem hiding this comment.
Idiomatic C# doesn't use _variable except in private getters/setters. Also, it's recommended that variables names are more explicit. In this case, I'd go with sampleDatabase or aggregationDatabase or similar.
There was a problem hiding this comment.
same as all cases below this, too.
There was a problem hiding this comment.
FWIW, the DB name comes from an existing code example. This is the DB name used across 5 pages across all of the programming languages, so changing it would be non-trivial and out of scope of this PR.
| public List<BsonDocument> PerformAggregation() | ||
| { | ||
| DotNetEnv.Env.TraversePath().Load(); | ||
| string uri = DotNetEnv.Env.GetString("CONNECTION_STRING", "Env variable not found. Verify you have a .env file with a valid connection string."); |
There was a problem hiding this comment.
usually use var instead of string unless it's not clear what is being returned.
| public void SeedData() | ||
| { | ||
| DotNetEnv.Env.TraversePath().Load(); | ||
| string uri = DotNetEnv.Env.GetString("CONNECTION_STRING", "Env variable not found. Verify you have a .env file with a valid connection string."); |
There was a problem hiding this comment.
usually use var instead of string unless it's not clear what is being returned.
| @@ -0,0 +1,3 @@ | |||
| { "CustomerId" : "oranieri@warmmail.com", "FirstPurchaseDate" : { "$date" : "2020-01-01T08:25:37Z" }, "TotalValue" : 63, "TotalOrders" : 1, "Orders" : [{ "OrderDate" : { "$date" : "2020-01-01T08:25:37Z" }, "Value" : 63 }] } | |||
There was a problem hiding this comment.
I don't know how this is used...should it be pretty-formatted?
There was a problem hiding this comment.
Just pulling it in from the docs page where it's currently used. We don't pretty format it there, so I kept the existing formatting. (Presumably because pretty formatting it would make it much longer, and that would take up a lot of real estate on the page, but I'm making an assumption.)
| 27017/tcp -> [::]:27017 | ||
| ``` | ||
|
|
||
| ### Create a .env file |
There was a problem hiding this comment.
Depending on my initial comment about using the 3-party library, this may change.
There was a problem hiding this comment.
Oh, good point. I've changed the way I'm loading this so it's now being used properly to load .env vars at the beginning of the test suite run instead of having to read the file over and over again in various places to get the vars. I think the third-party lib is justifiable for env var management, and according to NuGet, this one has 12 million users. I think it's probably safe in this case to assume it's reasonably maintained.
| [Test] | ||
| public void TestOutputMatchesDocs() | ||
| { | ||
| var obj = new GroupTotal(); |
There was a problem hiding this comment.
I'd have to run this to be sure, but I think you can make obj global, and then, because you are calling new GroupTotal() in the setup, you don't need to do it here. In fact, I think your setup isn't actually doing anything.
Also, please rename obj to something meaningful. A new developer will have no idea what it is.
| var results = obj.PerformAggregation(); | ||
|
|
||
| DotNetEnv.Env.TraversePath().Load(); | ||
| string solutionRoot = DotNetEnv.Env.GetString("SOLUTION_ROOT", "Env variable not found. Verify you have a .env file with a valid connection string."); |
There was a problem hiding this comment.
consider changing all string = to var =
| string fullPath = Path.Combine(solutionRoot, outputLocation); | ||
| var fileData = TestUtils.ReadBsonDocumentsFromFile(fullPath); | ||
|
|
||
| Assert.That(results.Count, Is.EqualTo(fileData.Length), "Result count does not match output example length."); |
There was a problem hiding this comment.
Consider adding the variables to the output, like:
`$"The result count {results.Count} does not match the expected count of {fileData.Length}."
| Assert.That(results.Count, Is.EqualTo(fileData.Length), "Result count does not match output example length."); | ||
| for (int i = 0; i < fileData.Length; i++) | ||
| { | ||
| Assert.That(fileData[i], Is.EqualTo(results[i]), $"Mismatch at index {i}: expected {fileData[i]}, got {results[i]}."); |
There was a problem hiding this comment.
ooo you did it here. Nice!
dacharyc
left a comment
There was a problem hiding this comment.
Thanks for the comments and suggestions! Anything else jump out at you here?
| // :uncomment-end: | ||
| // :snippet-end: | ||
| var client = new MongoClient(uri); | ||
| _aggDB = client.GetDatabase("agg_tutorials_db"); |
There was a problem hiding this comment.
FWIW, the DB name comes from an existing code example. This is the DB name used across 5 pages across all of the programming languages, so changing it would be non-trivial and out of scope of this PR.
| @@ -0,0 +1,3 @@ | |||
| { "CustomerId" : "oranieri@warmmail.com", "FirstPurchaseDate" : { "$date" : "2020-01-01T08:25:37Z" }, "TotalValue" : 63, "TotalOrders" : 1, "Orders" : [{ "OrderDate" : { "$date" : "2020-01-01T08:25:37Z" }, "Value" : 63 }] } | |||
There was a problem hiding this comment.
Just pulling it in from the docs page where it's currently used. We don't pretty format it there, so I kept the existing formatting. (Presumably because pretty formatting it would make it much longer, and that would take up a lot of real estate on the page, but I'm making an assumption.)
|
|
||
| ## To run the tests locally | ||
|
|
||
| ### Create a MongoDB Docker Deployment |
There was a problem hiding this comment.
It's a holdover from the other test suites - not specific to C#. But my recommendation is for us to test things locally unless Atlas is required, as local testing doesn't require spinning up cloud infrastructure.
| 27017/tcp -> [::]:27017 | ||
| ``` | ||
|
|
||
| ### Create a .env file |
There was a problem hiding this comment.
Oh, good point. I've changed the way I'm loading this so it's now being used properly to load .env vars at the beginning of the test suite run instead of having to read the file over and over again in various places to get the vars. I think the third-party lib is justifiable for env var management, and according to NuGet, this one has 12 million users. I think it's probably safe in this case to assume it's reasonably maintained.
This PR adds a proof-of-concept C# test suite, following a similar structure to our other test suites.
This PoC tests the C# code examples on the Group and Total Data page.