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

Adding unit test notebooks for calwebb_detector1 #63

Closed
wants to merge 3 commits into from

Conversation

aliciacanipe
Copy link
Collaborator

Before merging this PR, there are a couple issues that we have to discuss/resolve:

  1. I need to update the conftest.py in the jwst/ repo, because I wanted pytest to grab the docstrings for all the unit tests (this should be straightforward)
  2. There are some pytest dependencies that might not work in a normal pipeline build (ipython pytest-xdist pytest-html): https://github.com/spacetelescope/jwst#installing-for-developers
  3. I tried using tempfiles in the notebooks, but I'm not sure how it would work in practice when we execute the notebooks

So, let's hold off on trying to merge this PR for now.

@aliciacanipe
Copy link
Collaborator Author

aliciacanipe commented Jan 7, 2021

A comment for myself to remember to add the following to conftest.py in the jwst/ repo:

import pytest
import inspect

@pytest.mark.trylast
def pytest_configure(config):
    terminal_reporter = config.pluginmanager.getplugin('terminalreporter')
    config.pluginmanager.register(TestDescriptionPlugin(terminal_reporter), 'testdescription')

class TestDescriptionPlugin:

    def __init__(self, terminal_reporter):
        self.terminal_reporter = terminal_reporter
        self.desc = None

    def pytest_runtest_protocol(self, item):
        self.desc = inspect.getdoc(item.obj)

    @pytest.hookimpl(hookwrapper=True, tryfirst=True)
    def pytest_runtest_logstart(self, nodeid, location):
        if self.terminal_reporter.verbosity == 0:
            yield
        else:
            self.terminal_reporter.write('\n')
            yield
            if self.desc:
                    self.terminal_reporter.write(f'\n{self.desc} ')

@aliciacanipe aliciacanipe linked an issue Jan 27, 2021 that may be closed by this pull request
@aliciacanipe
Copy link
Collaborator Author

@york-stsci This was my draft PR for a set of unit test notebooks that I had on hold while the other issues were sorted. I think I should just be able to add the tmpdir cell to the notebooks and then we can merge these as well, right?

@york-stsci
Copy link
Collaborator

I think that they've already been added, so it might actually just make sense to close this PR. Unless there are test notebooks that are in this PR but haven't yet been added?

@aliciacanipe
Copy link
Collaborator Author

Yeah, these notebooks haven't been added yet, so I left the PR as a draft. But I need to add the tmpdir cells now that we have that in place.

@aliciacanipe aliciacanipe marked this pull request as ready for review February 12, 2021 18:43
@york-stsci
Copy link
Collaborator

So, there are a couple of other things that look as if they should be changed. If you look at the existing regression test notebooks, you'll see how I wrote the part of the script that runs the regression tests in those. As is, these notebooks will not run in the Jenkins environment, because the jwst module directory is not where they expect to find it, and also while the script creates a temporary directory, it then saves its output into a directory in the same directory as the notebook with the name "tmpdir", rather than using the temporary directory it defined. The already added regression tests also show how that works.

@aliciacanipe
Copy link
Collaborator Author

K sounds good, I'll work on them. Thank you!

@cracraft
Copy link
Collaborator

Since the unit tests were accidentally merged in a different PR, I'm closing this PR. Any updates to the unit tests will be opened as new PRs.

@cracraft cracraft closed this Jun 28, 2021
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.

Adding regression test notebooks on hold
3 participants