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

Added mocking in fido_search for test_noaa #2900

Merged
merged 26 commits into from Jan 25, 2019

Conversation

Projects
None yet
5 participants
@yashrsharma44
Copy link
Member

commented Jan 18, 2019

  1. Note that this is test commit, demonstrating the mocking of Fido.search and Fido.fetch
  2. Further tests using mock would be written after this pattern of mocking is useful

Description

Partial work for #2874

Added mocking in fido_search for test_noaa
1. Note that this is test commit, demonstrating the mocking of Fido.search
2. Further tests using mock would be written after this pattern of mocking is useful
@pep8speaks

This comment has been minimized.

Copy link

commented Jan 18, 2019

Hello @yashrsharma44! Thanks for updating the PR.

Line 18:1: E303 too many blank lines (3)
Line 25:20: E203 whitespace before ':'
Line 30:18: E231 missing whitespace after ':'
Line 31:19: E231 missing whitespace after ':'

Comment last updated on January 24, 2019 at 20:33 Hours UTC

@yashrsharma44 yashrsharma44 changed the title Added mocking in fido_search for test_noaa [WIP] Added mocking in fido_search for test_noaa Jan 18, 2019

@nabobalis nabobalis added this to the 1.0 milestone Jan 18, 2019

@nabobalis nabobalis changed the title [WIP] Added mocking in fido_search for test_noaa Added mocking in fido_search for test_noaa Jan 18, 2019

@sunpy-bot

This comment has been minimized.

Copy link

commented Jan 18, 2019

Thanks for the pull request @yashrsharma44! Everything looks great!

@sunpy sunpy deleted a comment from sunpy-bot bot Jan 18, 2019

@dpshelio dpshelio referenced this pull request Jan 19, 2019

Merged

remove mock dependency #2901

yashrsharma44 added some commits Jan 19, 2019

Added some more tests for test_noaa.py
1. Added a function, test_fetch_working, for checking if the server returns the same file
2. Used mocking to mock the remaining tests
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py Outdated
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py Outdated
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py Outdated
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py Outdated
Show resolved Hide resolved sunpy/net/dataretriever/tests/test_noaa.py Outdated

yashrsharma44 added some commits Jan 19, 2019

Some minor fixes
1. Exchanged the import statements
2. Added a new line for import statements
Changed the docstrings for the test
1. Also removed the redundant import
@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 19, 2019

Hey @nabobalis , what about the changelog? Do I need to add one?

@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 24, 2019

I can't understand why this is happening, as it is passing all the tests in my computer.
Also I think that == checks whether the two objects have the same content or not, so that should work out fine.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

I'm not sure either but if we can't get it to pass on the CI, I don't think we can merge it. I will run it locally and see if I can figure out anything.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

I just ran this locally as well and it did not pass.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

However, I wonder if the error is not actually your test.
I seem to get Data not Available in my return. I wonder if its actually a server issue.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

The output I have here:

(Pdb) qr1
<class 'sunpy.net.dataretriever.client.QueryResponse'><Table length=1>
     Start Time           End Time      Source  Instrument  Wavelength
       str19               str19         str4     str12        str3   
------------------- ------------------- ------ ------------ ----------
2012-10-04 00:00:00 2012-10-06 00:00:00   sdic noaa-indices        nan
(Pdb) mock_qr
<class 'sunpy.net.dataretriever.client.QueryResponse'><Table length=1>
     Start Time           End Time      Source     Instrument     Wavelength
       str19               str19         str4        str18           str3   
------------------- ------------------- ------ ------------------ ----------
2012-10-04 00:00:00 2012-10-06 00:00:00   sdic Data not Available        nan

The mock_qr seems to have the wrong instrument.

@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 24, 2019

Even after changing the input parameters, I am getting the same error :

map_ = {
        'Time_start': parse_time(start_date),
        'Time_end':  parse_time(end_date),
        'source': 'sdic',
        'instrument': 'noaa-indices',
        'physobs':'sunspot number',
        'provider':'swpc'
    }

@nabobalis can you have a look ?

@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 24, 2019

Another interesting thing I observed is two objects with same parameters are not turning out to be equal -

(Pdb) qr1 = LCClient.search(Time('2012/10/4', '2012/10/6'),Instrument('noaa-indices'))
(Pdb) qr2 = LCClient.search(Time('2012/10/4', '2012/10/6'),Instrument('noaa-indices'))
(Pdb) qr1==qr2
False
(Pdb) qr1 is qr2
False
(Pdb) assert qr1==qr2
*** AssertionError

So I can only compare the values of both the objects and not the object itself.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

I will have a look. I wonder if we need to make sure we have setup eq right (if we do at all).

The is check won't work since I think it compares memory objects which in that case will be different.

@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 24, 2019

So for this PR, shall I compare the remaining attributes?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

Ideally we should try to figure out how to get == to work. Since it would be useful for all the other tests.

@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 24, 2019

So we need to define the magic function for __eq__ and __ne__ in order to support the same syntax for other tests.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

Most likely. We would have to check.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

I think the issue is that the mock_qr is missing a time range.

@yashrsharma44

This comment was marked as outdated.

Copy link
Member Author

commented Jan 24, 2019

Regarding the above suggestion, it is still showing error -

-> assert mock_qr == qr1
(Pdb) n
AssertionError: assert <class 'sunpy...ces        nan == <class 'sunpy....ces        nan
  At index 0 diff: <sunpy.net.dataretriever.client.QueryResponseBlock object at 0x7ff949c45b00> != <sunpy.net.dataretriever.client.QueryResponseBlock object at 0x7ff949c45860>
  Full diff:
  <class 'sunpy.net.dataretriever.client.QueryResponse'><Table length=1>
  Start Time           End Time      Source  Instrument  Wavelength
  str19               str19         str4     str12        str3
  ------------------- ------------------- ------ ------------ ----------
  2012-10-04 00:00:00 2012-10-06 00:00:00   sdic noaa-indices        nan
> /home/yash/sunpydev/sunpy/sunpy/net/dataretriever/tests/test_noaa.py(56)test_fetch_working()
-> assert mock_qr == qr1
(Pdb) 

However, I can access the QueryResponseObject of the LCClient.search() , and then check the _map attribute, which contains all the data important for comparison ?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

Sure we can try that.

Added comparison based on internal attributes
1.  Currently, QueryResponseBlock does not support
    comparison operation, so this is a ad-hoc
    hack for comparing the values
@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 24, 2019

Doing:

    map_ = {
        'Time_start': parse_time(start_date),
        'Time_end':  parse_time(end_date),
        'TimeRange': TimeRange(parse_time(start_date), parse_time(end_date)),
        'source': 'sdic',
        'instrument': 'noaa-indices',
        'physobs': 'sunspot number',
        'provider': 'swpc'
    }

assert mock_qr[0]._map == qr1[0]._map

passed but I'm not a fan of accessing the _map like that.

@yashrsharma44

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

Yup, accessing private variables is not a good practice, but unless we add a __eq__ method, I guess it is difficult to access the internal values, as QueryResponse does not provide any public method/attribute to access it.

@nabobalis nabobalis dismissed stale reviews from Cadair and themself Jan 24, 2019

old news

@yashrsharma44

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

Update : I have used the public attributes of QueryResponseBlock, that should serve our purpose.

@Cadair

Cadair approved these changes Jan 25, 2019

@nabobalis nabobalis merged commit b26f68e into sunpy:master Jan 25, 2019

10 of 11 checks passed

sunpy.sunpy Build #20190124.19 has failed
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing de96a98...eb75f21
Details
codecov/project 86.52% (+11.31%) compared to de96a98
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

@yashrsharma44

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

Thank you guys for my first PR for Sunpy !!

ishanisri added a commit to ishanisri/sunpy that referenced this pull request Jan 28, 2019

yashrsharma44 added a commit to yashrsharma44/sunpy that referenced this pull request Feb 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.