Collect FaaS info during handshake #1156
Conversation
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Show resolved
Hide resolved
| createClientMetadataDocument(applicationName, driverInfo) == createExpectedClientMetadataDocument(applicationName, driverInfo) | ||
| } | ||
|
|
||
| def 'should create client metadata document and exclude the extra driver info if its too verbose'() { |
There was a problem hiding this comment.
These tests have been moved (altered where needed) to the java prose test.
There was a problem hiding this comment.
I would recommend converting these tests to Junit tests. Also keeping all the unit / non functional tests here instead of moving to the prose tests.
There was a problem hiding this comment.
Does this imply converting the groovy file to java?
I'm unsure about unit vs prose. Perhaps all of these tests should try to send hello at the end? The existing tests seemed to me very similar - like tests that should have been prose tests, but weren't specified whenever this was originally created.
There was a problem hiding this comment.
Yes, I would ditch any / all groovy spec tests.
I suppose sending hello isn't required to test the what this class generates which in my mind makes it more of a unit test. However, I have no issue with having them mixed in with prose tests and all being consistent.
There was a problem hiding this comment.
The issue with sending hello is it should be done sync and async - if the output is tested against.
If we can get this in driver-core that would be ideal.
| String original = System.getProperty(name); | ||
| System.setProperty(name, value); | ||
| runnable.run(); | ||
| System.setProperty(name, original); |
There was a problem hiding this comment.
For safety should this step be done in a try / finally block.
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Show resolved
Hide resolved
|
|
||
| import static java.lang.System.getenv; | ||
|
|
||
| public final class TestHelper { |
There was a problem hiding this comment.
Naming not great / very descriptive
There was a problem hiding this comment.
Does a better name come to mind? It's meant to be fairly generic, containing methods similar to what might be found in a test superclass.
There was a problem hiding this comment.
If they are all relating to one thing then I'd use that in the name - eg. EnvironmentTestHelper
There was a problem hiding this comment.
Turned this into a "WithWrapper" (removed repeat, which was unrelated). I considered creating some sort of junit annotation to remove the indentation, but that seemed more complicated.
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Show resolved
Hide resolved
| createClientMetadataDocument(applicationName, driverInfo) == createExpectedClientMetadataDocument(applicationName, driverInfo) | ||
| } | ||
|
|
||
| def 'should create client metadata document and exclude the extra driver info if its too verbose'() { |
There was a problem hiding this comment.
I would recommend converting these tests to Junit tests. Also keeping all the unit / non functional tests here instead of moving to the prose tests.
| * See <a href="https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#test-plan">spec</a> | ||
| * @see ClientMetadataHelperSpecification | ||
| */ | ||
| public class ClientMetadataHelperProseTest { |
There was a problem hiding this comment.
Should these tests be in driver-core and not in driver-sync?
As they are not driver specific I think probably not.
There was a problem hiding this comment.
Ah, right - before I settled on CollectionHelper (which I now see is in driver-core), I tried using a MongoClient, but one was only available in driver-sync.
There was a problem hiding this comment.
If they can all stay in driver-core that would be the ideal.
There was a problem hiding this comment.
I moved the prose test back to core - do let me know if you think anything else can be in a better place.
driver-core/src/test/functional/com/mongodb/client/TestHelper.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/ClientMetadataHelper.java
Show resolved
Hide resolved
|
@rozza I updated this PR with spec updates (see DRIVERS-2209) that should have been included in the original implementation. I confirmed that 935954b implies that "platform" is moved above env, when adding info in order (as this PR does). |
JAVA-4761