Skip to content

Conversation

@gerritholl
Copy link
Member

@gerritholl gerritholl commented May 4, 2020

Migrate FCI tests to pytest. This enables the more flexible set of
fixtures available there. This will be needed by at least one of the
tests to be added in #1177

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes flake8 satpy
  • Fully documented

Migrate FCI tests to pytest.  This enables the more flexible set of
fixtures available there.  This will be needed by at least one of the
tests to be added in pytroll#1177
@gerritholl
Copy link
Member Author

CodeFactor is wrong to complain here. PEP 8 prohibits a bare test of if len(x). It does not prohibit an explicit length test such as if len(x) > 0:

# Correct:
if not seq:
if seq:
# Wrong:
if len(seq):
if not len(seq):

Since pylint-dev/pylint#2815 (apparently not merged in the version CodeFactor uses), pylint reportedly no longer complains about explicit comparisons of len(x) > 0. Therefore, I propose this particular recommendation should be ignored.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM, just one typo.

@gerritholl
Copy link
Member Author

The test is failing because the previous test contained a bug, which I serendipitously fixed:

self.assertTrue(4, len(files))

This should have been assertEqual, and it would have failed, because we have len(files) == 5. In the new version it is

https://github.com/gerritholl/satpy/blob/7a8a121bce2a81e6af68652239f293edafde96cd/satpy/tests/reader_tests/test_fci_l1c_fdhsi.py#L256

which is correct.

gerritholl and others added 2 commits May 5, 2020 10:34
Fix a pre-existing typo in the caplog.text test (thanks @mraspaud).

Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
The FCI filenames matching test was buggy in multiple ways.  It was
passing six filenames and expecting four to match, even though five were
meeting the correct format.  This didn't fail in the past, because it
was using assertTrue rather than assertEquals.  Now that a previous
commit has fixed the assertion the test has started failing.  I don't
know why I ever expected 4 rather than 5 out of 6 filenames to pass,
this commit removes the one I was wrongly expecting to not pass from the
list of test filenames.
@coveralls
Copy link

coveralls commented May 5, 2020

Coverage Status

Coverage increased (+0.0004%) to 89.618% when pulling b51350e on gerritholl:fci-goes-pytest into 79ea923 on pytroll:master.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #1180 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1180   +/-   ##
=======================================
  Coverage   89.61%   89.61%           
=======================================
  Files         200      200           
  Lines       29522    29523    +1     
=======================================
+ Hits        26457    26458    +1     
  Misses       3065     3065           
Impacted Files Coverage Δ
satpy/tests/reader_tests/test_fci_l1c_fdhsi.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79ea923...b51350e. Read the comment docs.

This commit resolves a serious merge conflict (11 locations with
conflicting merge) after
https://github.com/pytroll/satpy/pull/1135/files was merged into master.
I hope I didn't break anything; tests and flake8 see to pass.
@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels May 5, 2020
@mraspaud mraspaud merged commit ef24f29 into pytroll:master May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:readers enhancement code enhancements, features, improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants