-
Notifications
You must be signed in to change notification settings - Fork 1.6k
enable disabled passed integration test #2120
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
Conversation
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.
lgtm, though I don't understand why the tests were disabled? It looks like they have explicit code to handle the undesired behaviour, so I suspect they would've always passed.
@@ -28,7 +28,7 @@ @implementation FSTTransactionTests | |||
|
|||
// We currently require every document read to also be written. | |||
// TODO(b/34879758): Re-enable this test once we fix it. | |||
- (void)xtestGetDocuments { | |||
- (void)testGetDocuments { |
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 recommend removing the TODO on the line above as it no longer makes sense (there's nothing to re-enable). There's already a TODO for this issue within the body (line 46)
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.
done
@@ -336,7 +336,7 @@ - (void)testUpdateTransactionally { | |||
|
|||
// We currently require every document read to also be written. | |||
// TODO(b/34879758): Re-enable this test once we fix it. | |||
- (void)xtestHandleReadingOneDocAndWritingAnother { | |||
- (void)testHandleReadingOneDocAndWritingAnother { |
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.
As above.
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.
done
@@ -425,7 +425,7 @@ - (void)testReadingADocTwiceWithDifferentVersions { | |||
|
|||
// We currently require every document read to also be written. | |||
// TODO(b/34879758): Add this test back once we fix that. | |||
- (void)xtestCannotHaveAGetWithoutMutations { | |||
- (void)testCannotHaveAGetWithoutMutations { |
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.
Similar to as above, but slightly different. In this test, there's no todo in teh body. So I'd recommend removing the TODO comment (like the others) but add in a TODO after line 440, similar to the first test.
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.
done
* Revert "Custom fdl domain (#2121)" This reverts commit ebea1ef. * Revert "enable disabled passed integration test (#2120)" This reverts commit 48e5f7a. * Revert "Wire together LRU GC (#1905)" This reverts commit 305f872. * Revert "Fix build under linux/gcc (#2116)" This reverts commit b0a4b6c. * Revert "Implement PatchMutation::ApplyToLocalView (#1973)" This reverts commit f66b5b5. * Revert "Add Firebase Source to Header Search Path (#2114)" This reverts commit 0540361.
Enable test cases. These three integration test cases are disabled. They are passing now in the sense the test case has modified consistent with the other platform. Although the bug in the TODO is still yet fixed and thus not removing that comment (it is possible once the TODO is fixed the logic in the test needs to be updated).