Issue 1943 - systematically testing pickling of astropy classes - #1947

NICE!

@taldcroft Good points. I've implemented them in my latest commit. Do you think the number of objects included should be expanded?

This is really cool! Out of curiosity, why are we not testing the values of the pickled object? Does a lot break if we do?

@mdboom I had been assuming that if an attribute could be pickled, then it was probably pickled correctly. I just added a fairly generic comparison step, and it hasn't broken anything so far.

@ktchrn - I was having trouble with table Column objects going through pickle and having incorrect attribute values (e.g. losing meta values), so it's definitely a good idea to test the values.

Before recursion, tests weren't catching objects with non-pickleable objects as attributes. Only big change so far is that coordinate systems break because they use Longitude.

This is great!

It looks like this should be combined with #1961, which includes a fix for Longitude

Agree this is awesome! Given that #1961 is already in the works, I agree with @astrofrog that it makes sense to rebase this against that once it's merged. But for the other failures, I wonder if maybe we should make new issues and then xfail them, so that this actually gets into the tests now?

@ktchrn: Now that #1961 is merged, I'd love to see this reinvigorated. It's a nice piece of work!

@mdboom Okay, I'll start working on it again. Are there any features that it particularly needs, apart from passing the Travis build with older numpy versions and no optional dependencies?

I think mainly we just need to get the tests passing. I did make one comment about a missing docstring as well.

For me the GitHub UI doesn't show a link to the travis-ci tests for the latest commit: b2405cc !?
Does anyone know where the build log is or how to start it for this PR?

@ktchrn - could you rebase this and address @cdeil's comment? Thanks!

@ktchrn - now that 0.4 is out we can consider merging this - could you rebase this and add a changelog entry to the 1.0 entry?

@ktchrn - sorry for the delay, could you rebase this so we can go ahead and merge it? Please include a changelog entry in the 1.0 section.

I don't think this really requires a changelog entry. It's just adding some new tests--users don't care about that.

Oops--what happened here? Did you try to rebase?

Yeah. I guess that's all stuff that has happened since the last time I worked on this. If it's more convenient, I can just open a new pull request starting from the project's current state.

@ktchrn - you should be able to clean this up by rebasing - what I suspect is that you may not have rebased against the latest upstream master. Assuming the astropy repo (not your fork) is upstream, you can do:

git fetch upstream
git rebase upstream/master

Let us know if there are any conflicts and you are not sure how to deal with them. Once the rebase is complete, you can force push to the same branch:

git push -f origin pickle-tests-issue-1943

Thanks! You're right, I was rebasing on my outdated master instead of upstream/master.

Great, that looks a lot better :)

Except of course for the build failing, but we'll get to the bottom of that. It's probably something minor.

Looks great--thanks @ktchrn !