Skip to content

Modernize database tests and cleanup #579

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

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Modernize database tests and cleanup #579

merged 4 commits into from
Feb 9, 2017

Conversation

SUPERCILEX
Copy link
Collaborator

@samtstern Since it looks like #481 isn't going to be merged anytime soon, this PR gives @CodingDoug credit for his work while allowing us to move forward with modernized tests.

CodingDoug and others added 3 commits January 16, 2017 20:46
Moved TestUtils into correct directory for its package.
Removed redundant tests.
Reformatted some code and renamed methods for clarity.
# Conflicts:
#	database/src/main/java/com/firebase/ui/database/FirebaseArray.java
@SUPERCILEX SUPERCILEX changed the title Modernize database test Modernize database tests Feb 7, 2017
@SUPERCILEX SUPERCILEX changed the title Modernize database tests Modernize database tests and cleanup Feb 7, 2017
Signed-off-by: Alex Saveau <[email protected]>

Cleanup and merge

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

Signed-off-by: Alex Saveau <[email protected]>
}

@Test
public void testPushIncreasesSize() throws Exception {
assertEquals(3, mArray.getCount());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was dropping this line intentional?

Copy link
Collaborator Author

@SUPERCILEX SUPERCILEX Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodingDoug removed it in his PR. @samtstern If you'd like I'll add it back (I'm not sure why he removed it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem a little strange to assert that the count after is "4" but not assert that it was "3" before. 4 is only correct if it's one more than the initial count.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is redundant. The '@before' annotated function gets run before every single '@test' function, and it verifies the size of the array ahead of time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodingDoug thanks for the explanation!

@samtstern
Copy link
Contributor

@CodingDoug any issues with merging in your test changes now since we all agree they are good? You can then rebase your PR and it will only contain the Map performance improvements.

@CodingDoug
Copy link
Contributor

@samtstern Yeah, that's fine. I promise I'll get back to the rest of this PR some time!

@samtstern samtstern merged commit b11cceb into firebase:version-1.2.0-dev Feb 9, 2017
@samtstern samtstern mentioned this pull request Feb 9, 2017
@SUPERCILEX SUPERCILEX deleted the db-tests branch February 9, 2017 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants