-
Notifications
You must be signed in to change notification settings - Fork 5
C# test suite PoC #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
||
public void SeedData() | ||
{ | ||
DotNetEnv.Env.TraversePath().Load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as all cases below this, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually use var
instead of string
unless it's not clear what is being returned.
@@ -0,0 +1,3 @@ | |||
{ "CustomerId" : "[email protected]", "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how this is used...should it be pretty-formatted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on my initial comment about using the 3-party library, this may change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo you did it here. Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" : "[email protected]", "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.