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

update tests for changes to pytest #820

Merged
merged 2 commits into from Jan 23, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Jan 20, 2024

opened on behalf of @aprsa

The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest.
f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue.
Copy link
Member Author

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

A little difficult to tease out the needed changes from everything else here, but I think everything looks reasonable. Just one general concern about removing the bundle returns since that has been helpful to debug failing tests in the past.

@@ -101,7 +97,6 @@ def test_21(verbose=False, plot=False):
if plot:
b.plot(show=True, time=0)

return b
Copy link
Member Author

Choose a reason for hiding this comment

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

are these intentionally removed (in several places throughout this PR)? They can be useful when running the script directly and debugging the bundle. If we do need to remove these, then the calls in __main__ need to be updated as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

(turns out this is necessary otherwise pytest yells, so we'll just need to manually make these edits if we need to inspect the bundle to debug a broken test).

Copy link
Contributor

@aprsa aprsa Jan 23, 2024

Choose a reason for hiding this comment

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

Yep, pytest currently raises a warning but the next version will raise an error whenever tests return anything. So we can't put those back in, unfortunately. See here for details.

theta = 0.9
area0 = 0.1554255703973858
volume0 = 5.7617852701434746e-03
# detacted case (only meaningful)
Copy link
Member Author

Choose a reason for hiding this comment

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

while we're changing other things 😝

Suggested change
# detacted case (only meaningful)
# detatched case (only meaningful)

@kecnry kecnry merged commit 4fdecc1 into phoebe-project:bugfix-2.4.13 Jan 23, 2024
10 checks passed
@kecnry kecnry deleted the fix-pytest branch January 23, 2024 14:50
@kecnry kecnry mentioned this pull request Mar 28, 2024
kecnry added a commit that referenced this pull request Apr 1, 2024
* Import from within source tree overly restrictive (#819)

In order to avoid same-name module conflicts with the system module (such as io.py), phoebe shouldn't be imported from its own source tree. The original test eliminated the entire package tree, including all auxiliary directories, which is overly restrictive and it prevented pytest from running when phoebe was installed with `pip -e`. This PR fixes that by blocking imports from the root directory and from the root/phoebe tree (the actual sources). Closes #806.

* update tests for changes to pytest (#820)

* Fixing up all pytests
The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest.

* Update f-string to be compatible with python 3.7

f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue.

---------

Co-authored-by: Andrej Prsa <aprsa09@gmail.com>

* Dynamical RVs now avoid meshing (#823)

* Dynamical RVs now avoid meshing
Calling b.compute_pblums() built the mesh and treated dynamical RVs as mesh-dependent. This fixes that for both run_compute() and for direct compute_pblums(), compute_l3(), and compute_ld_coeffs bundle methods. A new function, b._datasets_that_require_meshing(), filters datasets that require a mesh, i.e. 'lc', 'lp' and flux-weighted 'rv'. Closes #812.

* Generalized dataset filtering with b._datasets_where()

* Adding backend exception to mesh computation
Even if phoebe backend didn't need meshes for a particular dataset, other backends do. This is now handled in the `_datasets_where()` function.


---------

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

* run_checks_compute() bugfix (#837)

* run_checks_compute() bugfix
The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed.

* Incorporating @kecnry's comments into the PR (along with additional comments)

* Fixing the if statement per @kecnry's instructions.

* Fix treatment of distance for alternate backends (#832)

* regression test

* don't duplicate distance handling for alt backends

* temporarily skip jktebop

* use top-level default_binary in test

* version and changelog for 2.4.13 release

* update setup-python to v5

to avoid deprecation warning

---------

Co-authored-by: Andrej Prsa <aprsa@villanova.edu>
Co-authored-by: Andrej Prsa <aprsa09@gmail.com>
aprsa added a commit that referenced this pull request Apr 1, 2024
* Import from within source tree overly restrictive (#819)

In order to avoid same-name module conflicts with the system module (such as io.py), phoebe shouldn't be imported from its own source tree. The original test eliminated the entire package tree, including all auxiliary directories, which is overly restrictive and it prevented pytest from running when phoebe was installed with `pip -e`. This PR fixes that by blocking imports from the root directory and from the root/phoebe tree (the actual sources). Closes #806.

* update tests for changes to pytest (#820)

* Fixing up all pytests
The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest.

* Update f-string to be compatible with python 3.7

f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue.

---------

Co-authored-by: Andrej Prsa <aprsa09@gmail.com>

* Dynamical RVs now avoid meshing (#823)

* Dynamical RVs now avoid meshing
Calling b.compute_pblums() built the mesh and treated dynamical RVs as mesh-dependent. This fixes that for both run_compute() and for direct compute_pblums(), compute_l3(), and compute_ld_coeffs bundle methods. A new function, b._datasets_that_require_meshing(), filters datasets that require a mesh, i.e. 'lc', 'lp' and flux-weighted 'rv'. Closes #812.

* Generalized dataset filtering with b._datasets_where()

* Adding backend exception to mesh computation
Even if phoebe backend didn't need meshes for a particular dataset, other backends do. This is now handled in the `_datasets_where()` function.

---------

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

* run_checks_compute() bugfix (#837)

* run_checks_compute() bugfix
The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed.

* Incorporating @kecnry's comments into the PR (along with additional comments)

* Fixing the if statement per @kecnry's instructions.

* Fix treatment of distance for alternate backends (#832)

* regression test

* don't duplicate distance handling for alt backends

* temporarily skip jktebop

* use top-level default_binary in test

* version and changelog for 2.4.13 release

* update setup-python to v5

to avoid deprecation warning

---------

Co-authored-by: Andrej Prsa <aprsa@villanova.edu>
Co-authored-by: Andrej Prsa <aprsa09@gmail.com>
aprsa added a commit that referenced this pull request Apr 1, 2024
* Import from within source tree overly restrictive (#819)

In order to avoid same-name module conflicts with the system module (such as io.py), phoebe shouldn't be imported from its own source tree. The original test eliminated the entire package tree, including all auxiliary directories, which is overly restrictive and it prevented pytest from running when phoebe was installed with `pip -e`. This PR fixes that by blocking imports from the root directory and from the root/phoebe tree (the actual sources). Closes #806.

* update tests for changes to pytest (#820)

* Fixing up all pytests
The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest.

* Update f-string to be compatible with python 3.7

f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue.

---------

Co-authored-by: Andrej Prsa <aprsa09@gmail.com>

* Dynamical RVs now avoid meshing (#823)

* Dynamical RVs now avoid meshing
Calling b.compute_pblums() built the mesh and treated dynamical RVs as mesh-dependent. This fixes that for both run_compute() and for direct compute_pblums(), compute_l3(), and compute_ld_coeffs bundle methods. A new function, b._datasets_that_require_meshing(), filters datasets that require a mesh, i.e. 'lc', 'lp' and flux-weighted 'rv'. Closes #812.

* Generalized dataset filtering with b._datasets_where()

* Adding backend exception to mesh computation
Even if phoebe backend didn't need meshes for a particular dataset, other backends do. This is now handled in the `_datasets_where()` function.

---------

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

* run_checks_compute() bugfix (#837)

* run_checks_compute() bugfix
The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed.

* Incorporating @kecnry's comments into the PR (along with additional comments)

* Fixing the if statement per @kecnry's instructions.

* Fix treatment of distance for alternate backends (#832)

* regression test

* don't duplicate distance handling for alt backends

* temporarily skip jktebop

* use top-level default_binary in test

* version and changelog for 2.4.13 release

* update setup-python to v5

to avoid deprecation warning

---------

Co-authored-by: Andrej Prsa <aprsa@villanova.edu>
Co-authored-by: Andrej Prsa <aprsa09@gmail.com>
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

2 participants