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

Add test to see whether all Python files can be imported. #343

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

hugobuddel
Copy link
Collaborator

@hugobuddel hugobuddel commented Jan 17, 2024

This ensures that all files are included in the code coverage.

See #340 (comment)

The file mentioned there, https://github.com/AstarVienna/ScopeSim/blob/dev_master/scopesim/detector/nghxrg.py , is itself not broken (this test passes), but the detector package is:

$ python -c "import scopesim.detector"
Traceback (most recent call last):
[...]
  File "[...]/scopesim/optics/optical_train.py", line 19, in <module>
    from ..detector import DetectorArray
ImportError: cannot import name 'DetectorArray' from partially initialized module 'scopesim.detector' (most likely due to a circular import) ([...]/scopesim/detector/__init__.py)

Tests fail because now the coverage is down, which makes sense, because previously uncovered files are added. Turns out it was only scopesim/detector/nghxrg.py and scopesim/reports/report_generator.py though.

Needed to make some minor changes to get the test to pass.

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (44f5926) 77.04% compared to head (a942ef7) 75.52%.

Files Patch % Lines
scopesim/detector/nghxrg.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #343      +/-   ##
==============================================
- Coverage       77.04%   75.52%   -1.52%     
==============================================
  Files              57       59       +2     
  Lines            7693     7870     +177     
==============================================
+ Hits             5927     5944      +17     
- Misses           1766     1926     +160     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hugobuddel
Copy link
Collaborator Author

Oh, this change might mess things up, because it imports tst_ScopeSim_MET_LSS.py and that does

rc.__currsys__['!SIM.file.local_packages_path'] = r"F:/Work/irdb"

and thereby might make the tests order dependent etc. Perhaps everything would work fine if all the other tests are properly mocked, but still.

I propose to simply remove that file, because it doesn't really do anything that the IRDB is not also doing. It does profiling, but we can do that also in the IRDB, and perhaps in a generalized way.

So I'll do just that, remove the file.

The only useful thing in there is profiling; we should do that
in a more generic way. The test itself is covered by the IRDB.
@teutoburg
Copy link
Contributor

The one now failing test is again a 403. Which lead me to the discovery that the new "Tests with updated dependencies" uses pytest -m "webtest" instead of pytest -m "not webtest". I'll quickly fix that in the DevOps...

Copy link
Contributor

@teutoburg teutoburg left a comment

Choose a reason for hiding this comment

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

Certainly a useful addition 👍

@teutoburg teutoburg added the tests Related to unit or integration tests label Jan 17, 2024
@hugobuddel
Copy link
Collaborator Author

I noticed that still not all Python files were covered, so redid the logic once more.

This imported the broken profiling tests. Instead of fixing them, I decided to just delete them. We might need some profiling tests, but we should do that in a more generalized way. And then run them nightly on zeus or something and make plots of the timings over time. We can easily recreate the deleted file, it is a trivial setup.

Furthermore, I decided to add a time limit on importing the modules, of 200 milliseconds. This showed that test_database.py was accessing the web even when just importing the file (without running any tests). This has been fixed.

Could you perhaps review this again @teutoburg?

Sorry for the rerequest; I realized too late it was still not covering everything the previous time.

@teutoburg teutoburg self-requested a review January 17, 2024 18:00
Comment on lines +21 to +26
@pytest.fixture(scope="class")
def all_pkg():
# TODO: This could use some kind of mock to avoid server access
yield db.get_all_package_versions()


Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think I wanted to do somethings like that at some point, but then forgot 👍

@teutoburg
Copy link
Contributor

This showed that test_database.py was accessing the web even when just importing the file (without running any tests). This has been fixed.

Great, I already tried running the tests (not sure now if in scopesim itself or maybe ti was spextra) with -m "not webtest" locally with my internet connection turned off, and things failed. Didn't get around to investigating further. Not sure if this is possible in GitHub Actions, but I'd like a way of "simulating" an offline environment for the "not webtest"s, because IMO the point is for them to run without internet.

@hugobuddel hugobuddel merged commit 2c7a0f0 into dev_master Jan 17, 2024
22 of 24 checks passed
@hugobuddel hugobuddel deleted the hb/importall branch January 17, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Related to unit or integration tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants