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

Respectable testing for cmorizers/obs/utilities.py and cmorizers/obs/cmorize_obs.py #1517

Merged
merged 19 commits into from Apr 28, 2020

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Feb 11, 2020

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • Make sure your code is composed of functions of no more than 50 lines and uses meaningful names for variables
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #1516 and closes #1518

@valeriupredoi valeriupredoi changed the title Respectable testing for cmorizers/obs/utilities.py Respectable testing for cmorizers/obs/utilities.py and cmorizers/obs/cmorize_obs.py Feb 12, 2020
@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Feb 13, 2020

OK mucho better:

/home/valeriu/ESMValTool/esmvaltool/cmorizers/obs/cmorize_obs.py                                                  126     40    68%
/home/valeriu/ESMValTool/esmvaltool/cmorizers/obs/utilities.py                                                    187     18    90%

This is ready for review guys 🍺

@valeriupredoi valeriupredoi requested a review from bouweandela Feb 13, 2020
):
run()

os.chdir(curr_path)
Copy link
Member

@bouweandela bouweandela Feb 28, 2020

Choose a reason for hiding this comment

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

Suggested change
os.chdir(curr_path)

Stuff should work independent from what the current working directory is. Is this needed?

Copy link
Contributor Author

@valeriupredoi valeriupredoi Feb 28, 2020

Choose a reason for hiding this comment

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

afraid it is, due to the cmorizer entering subdirs

Copy link
Member

@bouweandela bouweandela Apr 20, 2020

Choose a reason for hiding this comment

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

Why is that a problem? Does some of the test code depend on the current working directory?

Copy link
Member

@bouweandela bouweandela Apr 28, 2020

Choose a reason for hiding this comment

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

Thanks for explaining. In that case you might want to use a context manager, to avoid writing the stuff to the wrong output directory if the test fails, e.g.

@contextlib.contextmanager
def keep_cwd()
    curr_path = os.getcwd()
    try:
        yield
    finally:
        os.chdir(curr_path)

and then run the test code with

with keep_cwd():
    test_something()

Copy link
Contributor Author

@valeriupredoi valeriupredoi Apr 28, 2020

Choose a reason for hiding this comment

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

legend, lemme do this now 🍺

valeriupredoi and others added 6 commits Feb 28, 2020
Co-Authored-By: Bouwe Andela <bouweandela@users.noreply.github.com>
Co-Authored-By: Bouwe Andela <bouweandela@users.noreply.github.com>
@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Mar 3, 2020

pk @bouweandela plugged in changes you suggested ma man - merge? 🍺

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Mar 3, 2020

hey @bouweandela there's still issues with the Julia installation pffft

--> actually prob not since these are run against the old task architecture before you changed it yesterday!

--> actually I just ran test_diagnostic_run.py against the latest esmvalcore and all goes through a-OK
--> beer time 🍺

Copy link
Member

@bouweandela bouweandela left a comment

Hi V,

Sorry to be so slow with looking at this pull request. It looks fine, just some minor comments. Could you merge the latest version of the master branch into this one, so we are sure things still work?

tests/unit/cmorizers/obs/test_utilities.py Outdated Show resolved Hide resolved
):
run()

os.chdir(curr_path)
Copy link
Member

@bouweandela bouweandela Apr 20, 2020

Choose a reason for hiding this comment

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

Why is that a problem? Does some of the test code depend on the current working directory?

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 20, 2020

cheers @bouweandela - I can't seem to be able to reply to your last comment in line (weird) - so the cmorizers performs an os.chdir() inside its operation getting inside the output dir and if the test is run it'll execute that operation and will perform all the other tests in that dummy dir and write the test reportes there, hence the need to bugger out that dir once the cmorizer test has finished

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 20, 2020

so this thing is failing tests with some really random stuff that I don't understand where they're coming from

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 20, 2020

ah, it's me who's random! bleh

@bouweandela bouweandela merged commit ecc25f0 into master Apr 28, 2020
6 checks passed
@bouweandela bouweandela deleted the improve_cmorizer_tests branch Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cmorization wrapper cmorize_obs.py is poorly tested Cmorization utilities script is poorly tested
2 participants