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

Faster, more consistent tests #740

Merged
merged 11 commits into from
Jun 9, 2016
Merged

Faster, more consistent tests #740

merged 11 commits into from
Jun 9, 2016

Conversation

abeisgoat
Copy link
Contributor

@abeisgoat abeisgoat commented Jun 1, 2016

@abeisgoat abeisgoat self-assigned this Jun 1, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 93.088% when pulling 37a2bc2 on test-improvements into df49c20 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.412% when pulling 21d3187 on test-improvements into df49c20 on master.

@abeisgoat abeisgoat changed the title [IGNORE] Faster, more consistent tests Faster, more consistent tests Jun 8, 2016
@abeisgoat
Copy link
Contributor Author

@jwngr Ptal - removed a couple uneeded tests, fixed the rest and sped everything up.

@abeisgoat abeisgoat removed their assignment Jun 8, 2016
});

it('only removes keys in query when query is used', function(done){
return ref.set(MOCK_DATA).then(function() {
tick();
// tick();
Copy link

Choose a reason for hiding this comment

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

I think this can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call ☎️

@jwngr
Copy link

jwngr commented Jun 8, 2016

Looks good to me although I think @katowulf has a better grasp on the digest loop stuff than I do. Tying it into our enableLogging() call seems fairly hacky but also fairly ingenious! The tests certainly do run significantly faster and seem to be much more stable. Great job!

https://www.youtube.com/watch?v=gTIcIzKjGjg

@jwngr jwngr assigned katowulf and unassigned jwngr Jun 8, 2016
@katowulf
Copy link
Contributor

katowulf commented Jun 8, 2016

Actually, Abe, I'm thinking your commit song should be this one.

@@ -114,14 +116,19 @@ describe('Todo App', function () {
});
return def.promise;
}).then(function () {
expect(todos.count()).toBe(6);
browser.wait(function() {
return element(by.id('todo-4')).isPresent()
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀🚀🚀
🔥🔥🔥

@katowulf
Copy link
Contributor

katowulf commented Jun 8, 2016

lgtm! Jacob and I both made minor nits. Decide if you want to address those and send it back my way to merge.

@katowulf katowulf assigned abeisgoat and unassigned katowulf Jun 8, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.412% when pulling d6e759e on test-improvements into df49c20 on master.

@abeisgoat abeisgoat assigned katowulf and unassigned abeisgoat Jun 9, 2016
@abeisgoat
Copy link
Contributor Author

Back to you Mr. W

@katowulf katowulf merged commit e36e652 into master Jun 9, 2016
@katowulf katowulf deleted the test-improvements branch June 9, 2016 14:05
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.

None yet

4 participants