Skip to content

Make max_width and max_lines deterministic for testing - #17177

Merged
pllim merged 3 commits into
astropy:mainfrom
astrofrog:deterministic-max-width-lines
Oct 15, 2024
Merged

Make max_width and max_lines deterministic for testing#17177
pllim merged 3 commits into
astropy:mainfrom
astrofrog:deterministic-max-width-lines

Conversation

@astrofrog

@astrofrog astrofrog commented Oct 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I decided to set the values to use for testing to the common default for a terminal rather than -1, because the latter requires some tests to be updated and also generates too large output in the docs. It is easier to set the values to the common defaults (80/24).

Fixes #8691
Fixes #16307

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@astrofrog
astrofrog requested a review from taldcroft as a code owner October 14, 2024 12:55
@astrofrog astrofrog added this to the v6.1.5 milestone Oct 14, 2024
@github-actions

github-actions Bot commented Oct 14, 2024

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • 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 instructions for rebase and squash.
  • 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. Codestyle issues can be fixed by the bot.
  • 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 this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • 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.

@astrofrog

astrofrog commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

mhvk
mhvk approved these changes Oct 14, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like a good solution to me!

pllim
pllim reviewed Oct 14, 2024
View reviewed changes
Comment thread astropy/table/tests/test_pprint.py Resolved
@astrofrog astrofrog changed the title Max max_width and max_lines deterministic for testing Make max_width and max_lines deterministic for testing Oct 14, 2024
@astrofrog @pllim
Improve context manager
cfdd2db 
Co-authored-by: P. L. Lim <[email protected]>
taldcroft
taldcroft approved these changes Oct 15, 2024
View reviewed changes

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@pllim
pllim merged commit 3da47a7 into astropy:main Oct 15, 2024
@pllim

pllim commented Oct 15, 2024

Copy link
Copy Markdown
Member

Thanks, all!

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Oct 15, 2024
@astrofrog @meeseeksmachine
Backport PR astropy#17177: Make max_width and max_lines deterministic…
13a0f4d 
… for testing
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 15, 2024
Merged
pllim added a commit that referenced this pull request Oct 15, 2024
@pllim
Merge pull request #17182 from meeseeksmachine/auto-backport-of-pr-17…
9124fdc 
…177-on-v6.1.x

Backport PR #17177 on branch v6.1.x (Make max_width and max_lines deterministic for testing)
@pllim pllim mentioned this pull request Oct 21, 2024
Closed
@bsipocz

bsipocz commented Oct 26, 2024

Copy link
Copy Markdown
Member

Can this, or a solution be ported into doctestplus? I sometimes see similar issues with other libraries when running the tests locally but I never dived deep enough to get a fix for them.

@astrofrog

astrofrog commented Oct 26, 2024

Copy link
Copy Markdown
Member Author

@bsipocz are the failures you see usually related to astropy.table or other packages too?

@bsipocz

bsipocz commented Oct 26, 2024

Copy link
Copy Markdown
Member

astropy table prints in astroquery and pyvo

@bsipocz

bsipocz commented Oct 26, 2024

Copy link
Copy Markdown
Member

See scientific-python/pytest-doctestplus#257

@astrofrog

astrofrog commented Oct 26, 2024

Copy link
Copy Markdown
Member Author

Ok - trying to think about how we could do this without special casing/importing astropy in the plugin. I guess another option would be for astropy to include a pytest 'plugin' that will configure/unconfigure this setting automatically?

@bsipocz

bsipocz commented Oct 26, 2024

Copy link
Copy Markdown
Member

Could it live in pytest-astropy?

@astrofrog

astrofrog commented Oct 27, 2024

Copy link
Copy Markdown
Member Author

That's a good idea!

@bsipocz

bsipocz commented Oct 27, 2024

Copy link
Copy Markdown
Member

Or in doctestplus with a conditional, but I think both packages use pytest-astropy for convenience, too

kkrutkowski pushed a commit to kkrutkowski/astropy that referenced this pull request Apr 13, 2025
@astrofrog @pre-commit-ci
@pllim @kkrutkowski
Make max_width and max_lines deterministic for testing (astropy#17177)
380c0b2 
* Max max_width and max_lines deterministic for testing, so that tests do not fail for e.g. large terminals

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Improve context manager

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: P. L. Lim <[email protected]>
@pllim pllim mentioned this pull request May 9, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@taldcroft taldcroft taldcroft approved these changes
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

no-changelog-entry-needed table testing

Projects

None yet

Milestone

v6.1.5

Development

Successfully merging this pull request may close these issues.

Doctest failure depending on terminal width 3.2rc1 Doctest failure in timeseries

5 participants

@astrofrog @pllim @bsipocz @taldcroft @mhvk