-
Notifications
You must be signed in to change notification settings - Fork 64
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
SWIFT-206 Use generated collection names for tests #181
Conversation
|
||
/// Generates a unique collection name of the format "-[<Test Case> <Test Name>]-<timestamp>". | ||
internal func generateCollectionName() -> String { | ||
return "\(self.name)-\(Date().timeIntervalSinceReferenceDate)" |
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.
Is there any risk of exceeding the 120-character limit when combining the database and collection name?
I'm not sure if a timestamp component on the collection name makes sense, as that means we no longer have deterministic collection names.
For some prior art, which inspired the original request for Swift, we do the following in PHP:
- In the low-level extension, we use the relevant portion of the path (basically a logical group name) and the test filename (test name) and replace any special characters with underscores: https://github.com/mongodb/mongo-php-driver/blob/master/tests/utils/tools.php#L396
- In the library, we use the short test name (e.g. test method) and append a short hash of the full name (method and class): https://github.com/mongodb/mongo-php-library/blob/master/tests/TestCase.php#L148
In both cases, we can trust that each test will use the same collection on successive test suite runs. When a test succeeds, we clean up the collection(s) under test, but leave them as-is after a failure so the data can be inspected if needed.
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.
The risk is definitely there, but I just didn't think it was likely enough to worry about. I originally didn't want to hash them because it would be harder to manually inspect after a failure, but your second method (combining the test name plus a hash) seems to fix that issue.
Also, I misunderstood what this ticket was going for when I added the timestamp. I included it because I thought we wanted to have unique collection names for every run of the test (so a stopped test run wouldn't require cleanup and past failures wouldn't influence future attempts).
Anyways, I think I will go with the naming scheme in the library (or something 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.
I ended up doing basically what the low level extension did, except instead of using the path, I used the test case name (file name) and the test name (function name). It turns out that hashing strings in Swift does not have great standard library support, and I wouldn't be able to do it here without involving some ObjC stuff.
To distinguish between collections within the same test, I appended something to the end, usually keeping with what was there before (e.g. a number, or a filename plus number in the case of crud tests).
@@ -14,7 +14,7 @@ class MongoSwiftTestCase: XCTestCase { | |||
|
|||
/// Gets the name of the database the test case is running against. | |||
internal class var testDatabase: String { | |||
return "test" | |||
return String(describing: self) |
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 also changes the database used for each case to simply be the name of the test case, instead of "test" for everything.
On a related note to https://github.com/mongodb/mongo-swift-driver/pull/181/files#r244839841, I'm not sure if using different database names across the test suite is an improvement. There is definitely more overhead to creating a database than a collection, as each database has its own file(s). Consolidating the test suite to a single database name when possible (some spec tests may require their own names) makes it easier to clean up data that is left behind. Looking again to the PHP library, which inspired this request, we use "test" by default but allow the user to override this with an environment variable or test config option. I'm not sure if the latter exists in Swift, since we're using XCTest here, but I'd think an environment variable is possible.
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 seems I misunderstood this line from the ticket description as well. I took "the test suite" to mean a single test file, e.g. DocumentTests
, hence this change. I'll simply revert it, and if we want to add test environment configuration I can follow up in another ticket.
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.
one nit, lgtm otherwise
SWIFT-206
This PR adds a function to
MongoSwiftTestCase
for generating collection names of the format "<Test Case>_<Test Name>_<optional suffix>
".e.g. a namespace for
testListDatabases
would betest.MongoClientTests_testListDatabases