Skip to content
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

Remove unnecessary rmq database operations in unit tests. #4236

Merged
merged 21 commits into from Nov 7, 2019

Conversation

@chliangGoogle
Copy link
Contributor

chliangGoogle commented Nov 5, 2019

DB operations between different test class that access the same database sometimes cause race conditions. Minimize DB initialization and operations to see if this resolves the issue.

@googlebot googlebot added the cla: yes label Nov 5, 2019
@maksymmalyhin

This comment has been minimized.

Copy link
Contributor

maksymmalyhin left a comment

Thank you for taking care of it!

Example/Messaging/Tests/FIRInstanceIDWithFCMTest.m Outdated Show resolved Hide resolved
_testUtil = [[FIRMessagingTestUtilities alloc] initWithUserDefaults:defaults withRMQManager:YES];
_mockMessaging = _testUtil.mockMessaging;
_mockInstanceID = _testUtil.mockInstanceID;
_messaging = _testUtil.messaging;

This comment has been minimized.

Copy link
@maksymmalyhin

maksymmalyhin Nov 5, 2019

Contributor

Maybe just remove _mockMessaging, _mockInstanceID, etc. and refer to _testUtil directly to avoid potential confusion in the future?

This comment has been minimized.

Copy link
@chliangGoogle

chliangGoogle Nov 6, 2019

Author Contributor

this gonna be a big refactor, will probably do it in another CL.

This comment has been minimized.

Copy link
@chliangGoogle

chliangGoogle Nov 6, 2019

Author Contributor

This is done in this PR.

@chliangGoogle

This comment has been minimized.

Copy link
Contributor Author

chliangGoogle commented Nov 6, 2019

xcode 10.2 seems to start failing but with some unrelated errors

Copy link
Member

paulb777 left a comment

LGTM

Please update .travis.yml to make the tests required again. see here.

Working on the Xcode 10.2 issue in #4254

@chliangGoogle

This comment has been minimized.

Copy link
Contributor Author

chliangGoogle commented Nov 7, 2019

Failure is unrelated to Messaging. Also couldn't find the restart the test button anymore. I just submit the code for now.

@chliangGoogle chliangGoogle merged commit 54b1458 into master Nov 7, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
cla/google All necessary CLAs are signed
danger/danger All green. Congrats.
kokoro Kokoro build finished
Details
@chliangGoogle chliangGoogle deleted the fcm-master-break branch Nov 7, 2019
@firebase firebase locked and limited conversation to collaborators Dec 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.