search

EliAndrewC / sideboard

BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

fixes #8 #39

Closed EliAndrewC closed 10 years ago

EliAndrewC commented 10 years ago

Changes are listed/explained in #8.

Since this is split across several commits, I probably should have done like a git rebase to edit the commit history to merge these into one big commit. I kind of figures that there'd be a single diff to review here; I guess that's one thing that Crucible dos automatically that Github doesn't.

robdennis commented 10 years ago

if you want to (it is a pretty length PR), I guess it's still possible to make this a rebased PR: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

robdennis commented 10 years ago

have some questions on the first 3 that you're answering. I'm in transit tomorrow, so not super available.

Personally, I'd want to see if there was a way for travis to run these tests before merging now that #38 is ready to turn on travis. This is not a hard requirement, but if it's doing, I think it'd be a nice thing to have in the PR conversation chain

EliAndrewC commented 10 years ago

Seems reasonable, I'm not in any hurry to get this merged, and I can follow those directions you linked me to elsewhere to enable the Travis webhook and get that resolved before merging this pull request. I need to write some more documentation anyway.

robdennis commented 10 years ago

confirming that travis passing, as we discussed we'll merge this one in.

EliAndrewC commented 10 years ago

There does seem to be a sporadic failure in the unit tests, it looks like sometimes the services don't get cleaned up properly so we end up calling into a method that doesn't exist. I'm not sure why or how that would happen, but I do sometimes see the unit tests fail with a single test case failure (test_unsubscribe) because of it. Not something that should have held up merging this pull request, but something I'll keep an eye out for.

I've also seen test_client_locking fail sometimes, which implies that the locking isn't actually consistent enough to be reliable, or that there's a bug in the test. I'll take a look when I get a chance, maybe open a ticket for these if we keep seeing them fail sporadically.