Skip to content

TST: Add regression test for Issue 12706#12714

Merged
mhvk merged 2 commits intoastropy:mainfrom
pllim:add-tst-table-grp-f8
Jan 11, 2022
Merged

TST: Add regression test for Issue 12706#12714
mhvk merged 2 commits intoastropy:mainfrom
pllim:add-tst-table-grp-f8

Conversation

@pllim
Copy link
Member

@pllim pllim commented Jan 10, 2022 •
edited
Loading

Description

This pull request is to add a regression test. Might want to backport because we should also guard LTS against this creeping back in again.

Fixes #12706 and fixes #12687

TODO

  • Do we need to pin numpy version? Only do it if it fails.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@pllim pllim added this to the v5.0.1 milestone Jan 10, 2022
@pllim pllim mentioned this pull request Jan 10, 2022
Closed
@github-actions github-actions bot added the table label Jan 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 10, 2022

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

mhvk
mhvk approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Great!

@mhvk
Copy link
Contributor

mhvk commented Jan 11, 2022

Except of course that it fails: I think we'd want to do an xfail on numpy == 1.22.0 (probably need to define a NUMPY_LT_1_22_1 for that...)

@pllim
Copy link
Member Author

pllim commented Jan 11, 2022

Hmm, I did a skipif instead of xfail. Do you want me to change it to xfail?

@pllim pllim marked this pull request as ready for review January 11, 2022 15:12
@pllim
Copy link
Member Author

pllim commented Jan 11, 2022

"Allowed failure" job failed with an unrelated socket timeout.

mhvk
mhvk approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Looks good! Given that we know 1.22 gives a failure, skipif is more logical - it is not as if the test will suddenly surprisingly succeed!

@mhvk mhvk merged commit dd5b594 into astropy:main Jan 11, 2022
@mhvk
Copy link
Contributor

mhvk commented Jan 11, 2022

Thanks, @pllim!

@lumberbot-app
Copy link

lumberbot-app bot commented Jan 11, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v5.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 dd5b594334c43ff504e035d94f3e5ed017ff1d14
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #12714: TST: Add regression test for Issue 12706'
  1. Push to a named branch:
git push YOURFORK v5.0.x:auto-backport-of-pr-12714-on-v5.0.x
  1. Create a PR against branch v5.0.x, I would have named this PR:

"Backport PR #12714 on branch v5.0.x (TST: Add regression test for Issue 12706)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@pllim pllim deleted the add-tst-table-grp-f8 branch January 11, 2022 16:02
pllim pushed a commit to pllim/astropy that referenced this pull request Jan 11, 2022
@mhvk @pllim
Merge pull request astropy#12714 from pllim/add-tst-table-grp-f8
c15fba2 
TST: Add regression test for Issue 12706
@pllim pllim mentioned this pull request Jan 11, 2022
Merged
9 tasks
@pllim
Copy link
Member Author

pllim commented Jan 11, 2022

Manual backport done in #12719

pllim added a commit that referenced this pull request Jan 11, 2022
@pllim
Merge pull request #12719 from pllim/backport-pr12714
14fbebb 
Backport PR #12714 on branch v5.0.x (TST: Add regression test for Issue 12706)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes

+1 more reviewer

@lpsinger lpsinger lpsinger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug no-changelog-entry-needed table testing

Projects

None yet

Milestone

v5.0.1

Development

Successfully merging this pull request may close these issues.

Astropy grouped table aggregation with columns of non-native endianness is broken in Numpy 1.22 TST: Unpin conditions in test_copy_vla

3 participants

@pllim @mhvk @lpsinger