Tests for scripts in io/fits/scripts by mohanagr · Pull Request #5858 · astropy/astropy

mohanagr · 2017-03-03T16:27:27Z

Implements test for script fitsheader as of now.

Reference:

mohanagr · 2017-03-03T16:39:00Z

Table formatting is yet to be tested. Working on that.

MSeifert04 · 2017-03-03T16:39:47Z

I think some unrelated changes (commits) made it into this PR. Could you clean them up (remove them) so it only contains the "new" changes?

mohanagr · 2017-03-03T16:42:52Z

Oh. My bad. Sorry. Those commits will make the builds fail. Fixing.

Edit: @MSeifert04 Done

MSeifert04 · 2017-03-03T16:55:19Z

+
+    @pytest.mark.parametrize('test_file', [
+        ('test0.fits'),
+        pytest.mark.xfail(('random.fits'), reason = 'bad file')


Why create a test just to xfail it? Or am I misunderstand this line?

There are two tests, one is supposed to pass i.e. test0.fits does exist and other xfails. I thought I should check if providing only one arg works correctly.

But a test that should fail shouldn't be xfail but tested with pytest.raises?

I agree with @MSeifert04. xfail shouldn't be used for this case, if a failure is expected and normal behaviour then it should be caught with pytest.raises.

@MSeifert04 @bsipocz All right. What are the use cases for xfail then?

xfail - should work but currently doesn't because of some unfixed bug (either in-package or upstream).

skip - doesn't work if dependency is too old or if dependency is not installed

pytest.raises - should fail, make sure it fails with the right exception (and exception message).

That's not official, that's just how I interpret these.

MSeifert04 · 2017-03-03T16:56:34Z

+    # Different test because stderr will not be none in case of testing by existing keyword
+    # fitsheader searches for a keyword in all the HDU's
+    @pytest.mark.parametrize('test_kw', [
+        ('RANDOMKEY')


This looks like only one test? It doesn't make sense to actually parametrize the test, right?

MSeifert04 · 2017-03-03T17:00:05Z

+#     with open(request.params[0]) as test_fits:
+#         yield test_fits
+
+@pytest.fixture(scope='module')


Is it actually necessary to declare this helper-function as fixture?

Using fixture I wouldn't have to call the function everytime. (lesser executions because module scope? lots of method use it).

So this is just so the regex doesn't need to be compiled multiple times?

Yes. module scope fixtures distribute the return value across functions which need it, don't they? Although I know it's almost no overhead. Should I remove it and replace by functions?

Well they seem like a really complicated way (using regex) of testing if str.startswith('WARNING') or 'WARNING' in str, right?

I just thought RE might be of use later. I'll change it if required.

I agree with @MSeifert04 . This seems unnecessarily complicated. Using startswith() is more readable.

MSeifert04 · 2017-03-03T17:47:52Z

It's my personal opinion but I wouldn't test any expected failures, like specifying a non-existing file, keyword or extensions.

I think we basically need to make sure the script works and doesn't get accidentally broken. The failures (probably) come from the non-scripty part of the io.fits module and should be tested already there.

mohanagr · 2017-03-03T18:16:03Z

Doesn't script not emitting warnings when it should come under accidentally broken (i.e. bad user inputs and things)?

MSeifert04 · 2017-03-03T18:25:26Z

@mohanagr Before you push anything else: I would be very interested to see what travis+coverage reports.

I really need to think a bit more about the scope of the PR and what to test with scripts. In the end some utility that can be used to test scripts would be really nice. Something like you basically did with your setup and teardown but more dynamic so it can be easily adjusted for other scripts.

Let's wait and see what the others say.

mohanagr · 2017-03-03T18:29:56Z

All right. I'd really like to contribute more to this issue and the testing part in general. Guess that'd help me slowly understand the codebase better, so that I can tackle other issues as well.

mohanagr · 2017-03-03T18:38:58Z

@MSeifert04 I got your point. The cases for invalid files, extensions etc. has been handled in the non-script part of the script! I'll remove the xfail ones in the next commit once Travis finishes builds.

pllim

I wonder if testing command line need to be this detailed. The command line is basically a short cut to codes that are already tested elsewhere. IMHO, just testing that the command line arguments work as intended for the most common cases is enough; there is no need for all the xfail and raises, no?

pllim · 2017-03-03T19:48:13Z

+#     with open(request.params[0]) as test_fits:
+#         yield test_fits
+
+@pytest.fixture(scope='module')


I agree with @MSeifert04 . This seems unnecessarily complicated. Using startswith() is more readable.

pllim · 2017-03-03T19:50:11Z

+
+    @pytest.mark.parametrize('test_kw,expected', [
+        ('BSCALE', 'BSCALE  =           1.000000E0 / REAL = TAPE*BSCALE + BZERO'),
+        pytest.mark.xfail(('LRFWAVE', 'LRFWAVE = 0.0 / linear ramp filter wavelength'), reason = 'bad format')


Putting xfail inside parametrize is hard to read. I prefer xfail to be its own thing.

pllim · 2017-03-03T19:51:38Z

+class TestFITSheader_script(FitsTestCase):
+
+    def setup_method(self,  method):
+        self.sys_argv_orig = sys.argv


Using subprocess to spawn the command line would be simpler but I wonder if that would work in Windows; do you know, @MSeifert04 ?

There should be no problem with subprocess, I've used it several times (mostly to call zlib). However, is coverage able to deal with subprocesses?

Good point. Maybe this is not worth pursuing.

@pllim subprocess does work on windows AFAIK. Also I will remove the xfail things. I understand the usage better now. I will add the test for the table formatting and go on with fitscheck.

saimn · 2017-03-03T21:46:27Z

It is possible to avoid mocking sys.argv by having the main function take an optional args argument which is passed to ArgumentParser.parse_args. This is already possible with the fitsheader script: https://github.com/astropy/astropy/blob/master/astropy/io/fits/scripts/fitsheader.py#L284 (and easy to do for the other scripts).

About the way to test scripts, some options:

pytest has to TestDir.run method to test itself it seems, in a subprocess or not. http://stackoverflow.com/a/13500346/4728722 gives an example.
there is also https://pypi.python.org/pypi/pytest-console-scripts

Just some thoughts, I'm not sure it's useful to put too much work here.

mohanagr · 2017-03-04T04:12:06Z

@saimn @pllim In my opinion monkeypatch is the best thing to do. It allows one to change context for a test and then go back. (Like argv, pwd, syspath etc.). Simple enough.

pllim · 2017-03-06T15:39:50Z

For the sake of simplicity, I think the current call to main() should be okay...

mohanagr · 2017-03-06T18:07:49Z

@pllim Sorry I read your comment after pushing. I have taken other comments into account.

Removed xfails
Fixed need to mock sys.argv
I'm deliberately including a few tests where the script should fail by making assertions on output (instead of raising an error myself and using pytest.raises) I'll remove those too if necessary.

MSeifert04 · 2017-03-06T18:10:30Z

@mohanagr Just out of interest: What is the motivation for the tests you created? Did you create tests based on (missing) coverage? Or because of use-cases you had?

This is not meant as criticism, I'm just curious 😅

mohanagr · 2017-03-06T18:13:34Z

@MSeifert04 Not really. But are you talking about coverage in this particular file?

Edit :- I am fairly new to testing and actually wrote the tests based on the usecases I could understand.

mohanagr · 2017-03-06T18:14:19Z

Also can you suggest how to test fitscheck? I'll manually run that script and compare outputs in my tests?
@MSeifert04 some help?

saimn · 2017-03-07T21:28:37Z

@saimn @pllim In my opinion monkeypatch is the best thing to do.

IMO it brings nothing here and only adds useless complexity, as you can use directly fitsheader.main([...]) and it will work the same.

mohanagr · 2017-03-12T14:49:17Z

Can someone please tell me how to go ahead with #5874 ? Should I push the fix here? Because it will hamper the testing (one of the usecases of the fitscheck script).

pllim · 2017-03-13T14:54:19Z

Ideally, the bug fix would be a separate PR and should be merged first, and then this PR to be rebased on top of that once merged. They have to be separate because this PR would go into v2.0 and the bug fix into v1.3.2. Hope this clarifies the matter. I apologize for any inconvenience caused.

mohanagr · 2017-03-13T14:57:10Z

@pllim Yes. Thanks. I'll just file a separate PR too. I pushed here so that I could continue testing this. Or I'll to omit one test case.
P.S. There is really no inconvenience really. Things for me to learn.

pllim · 2017-03-13T15:01:11Z

@mohanagr , the affected test can move to the new bug fix PR as well, if possible. This is because usually we ask the bug fix PR to add a test to demonstrate that the bug is indeed fixed and to prevent it from creeping back in the future.

saimn · 2017-05-15T10:13:23Z

@mohanagr - Are you still interested in this ? If yes, to make progress, I would suggest to focus here on test_fitsheader.py, and to limit the number of things to test.

mohanagr · 2017-05-15T14:30:46Z

Yes, I'll continue this and try to fix the test scripts. On May 15, 2017 3:43 PM, "Simon Conseil" <[email protected]> wrote: @mohanagr <https://github.com/mohanagr> - Are you still interested in this ? If yes, to make progress, I would suggest to focus here on test_fitsheader.py, and to limit the number of things to test. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5858 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOc7f9pA6cCfWYWUvBbn0JOjiqi3EN7uks5r6CVIgaJpZM4MSdgI> .

saimn · 2017-05-15T20:53:35Z

@mohanagr - Ok, great. So I think you should start with testing the normal usage of the script: choose a test file and simply test that the output is the expected one (with or without -e).
With a test file whose header is not too long (I would choose astropy/io/fits/tests/data/table.fits) you can directly compare the output with the expected one. The goal is really to test the script, not the behavior of fits.Header which is already well tested. So there is no real need for all the pytest magic here (test one keyword with -k and remove the parametrize uses, remove monkeypatch as explained above), and keep the errors testing and edge cases for the end.

saimn · 2017-05-15T20:22:59Z

+from ..hdu import PrimaryHDU
+from ..scripts import fitsheader
+from ....tests.helper import catch_warnings
+from ....tests.helper import pytest


Could you rebase on master, and import pytest directly (the bundle version was removed recently).

saimn · 2017-05-15T20:27:41Z

+import re
+
+from . import FitsTestCase
+from ..hdu import PrimaryHDU


Please remove the imports that are not used (PrimaryHDU, AstropyDeprecationWarning).

astropy-bot · 2017-09-27T12:16:10Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 6 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please close this and open an issue instead to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If you believe I commented on this issue incorrectly, please report this here.

pllim · 2017-09-27T15:42:43Z

This was created during GSoC 2017 application period, which is now over. @mohanagr , if you are interested to finish this, feel free to re-open, but I am closing this for now due to inactivity.

mohanagr added 2 commits March 3, 2017 22:18

Adds initial tests for fitsheader script

92c4c45

Remove unecessary comments

e5153e2

mohanagr force-pushed the TestFitsHeader branch from d9c8b83 to e5153e2 Compare March 3, 2017 16:50

MSeifert04 reviewed Mar 3, 2017

View reviewed changes

Remove parametrize for test with single test_arg

a262d72

pllim added io.fits testing labels Mar 3, 2017

pllim reviewed Mar 3, 2017

View reviewed changes

Fixes the need to mock sys.argv and removes xfails

6c61675

This was referenced Mar 8, 2017

Split pytest plugins out into individual modules #5770

Closed

script 'fitscheck' not deleting CHECKSUM and DATASUM keywords #5874

Closed

mohanagr force-pushed the TestFitsHeader branch from 833fc3e to 6c61675 Compare March 14, 2017 13:53

saimn requested changes May 15, 2017

View reviewed changes

pllim closed this Sep 27, 2017

saimn mentioned this pull request Sep 27, 2017

Add tests for Fits scripts #6621

Merged

Conversation

mohanagr commented Mar 3, 2017

Reference:

Uh oh!

mohanagr commented Mar 3, 2017

Uh oh!

MSeifert04 commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohanagr commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohanagr Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohanagr commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 commented Mar 3, 2017

Uh oh!

mohanagr commented Mar 3, 2017

Uh oh!

mohanagr commented Mar 3, 2017

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saimn commented Mar 3, 2017

Uh oh!

mohanagr commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

MSeifert04 commented Mar 3, 2017 •

edited

Loading

mohanagr commented Mar 3, 2017 •

edited

Loading

mohanagr Mar 3, 2017 •

edited

Loading

MSeifert04 Mar 3, 2017 •

edited

Loading

MSeifert04 Mar 3, 2017 •

edited

Loading

MSeifert04 Mar 3, 2017 •

edited

Loading

MSeifert04 Mar 3, 2017 •

edited

Loading

MSeifert04 commented Mar 3, 2017 •

edited

Loading

mohanagr commented Mar 3, 2017 •

edited

Loading

mohanagr commented Mar 4, 2017 •

edited

Loading

mohanagr commented Mar 6, 2017 •

edited

Loading

MSeifert04 commented Mar 6, 2017 •

edited

Loading

mohanagr commented Mar 6, 2017 •

edited

Loading

mohanagr commented Mar 6, 2017 •

edited

Loading

mohanagr commented Mar 12, 2017 •

edited

Loading

pllim commented Mar 13, 2017 •

edited

Loading