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

test: update the test suit #635

Merged
merged 6 commits into from
Nov 28, 2022
Merged

test: update the test suit #635

merged 6 commits into from
Nov 28, 2022

Conversation

12rambau
Copy link
Member

@12rambau 12rambau commented Nov 28, 2022

context

It all started when I realized 2 things:

  1. the earthengine tests are not run in the github CD/CI
  2. the earthengine tests are all relying on my account and a sepal_ui folder within it

objectives

The main objectives of this huge PR are:

  • make sure that all the tests are run in the test suit whatever the environment
  • run the test on local earthengine account without the support of my sepal-ui folder
  • increase test speed
  • improve the display of the unit tests

content

In the end I did way much more things than expected, I'll try to be as exhaustive as possible to make the review easier:

  • following these recomandation I set up an EARHENGINE_TOKEN in the Github action secrets and make sure init_ee could pick it up.
  • refactor conftest.py so that it creates all the needed assets in a specific folder
  • use a hex _hash for all the assets so that tests can be run in parallel. if not some names would be conflicting everywhere.
  • update the tests so that we check for ee.credentials to skip gee-related tests
  • split all mixed tests in aoi_model and reclassify_model so that gee and local behavior are tested separately
  • make good use of the scope of the pytest fixture to speed up file creation (by avoiding duplication)
  • as recommended in this article I added pytest-sugar, pytest-icdiff and pytest-instafail (only used in the github workflow)
  • remove nearly all calls to deprecated methods from our lib and other (I opened some new issues about that while making the modification e.g. leaflet add method)
  • global speed boost
  • create a test_decorator.py file
  • update the contribution section with the earth engine ENV setup

side notes

related issues

they are never completely solved but that's a good introduction
#562
#621
#626
#630

conclusion

Happy to hear feedback but as it's not changing any of the lib behavior I'll try to merge it as soon as possible to start follow-up PR

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #635 (3303921) into main (7005078) will decrease coverage by 0.33%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##             main     #635      +/-   ##
==========================================
- Coverage   92.97%   92.63%   -0.34%     
==========================================
  Files          36       36              
  Lines        3913     3911       -2     
==========================================
- Hits         3638     3623      -15     
- Misses        275      288      +13     
Impacted Files Coverage Δ
sepal_ui/scripts/utils.py 90.34% <0.00%> (-2.16%) ⬇️
sepal_ui/aoi/aoi_view.py 98.81% <94.73%> (+0.07%) ⬆️
sepal_ui/aoi/aoi_model.py 89.31% <100.00%> (ø)
sepal_ui/mapping/aoi_control.py 100.00% <100.00%> (ø)
sepal_ui/mapping/draw_control.py 97.56% <100.00%> (ø)
sepal_ui/mapping/sepal_map.py 90.39% <100.00%> (-0.58%) ⬇️
sepal_ui/mapping/value_inspector.py 88.99% <100.00%> (ø)
sepal_ui/reclassify/reclassify_model.py 92.36% <100.00%> (-1.89%) ⬇️
sepal_ui/reclassify/reclassify_view.py 84.46% <100.00%> (ø)
sepal_ui/reclassify/table_view.py 59.45% <100.00%> (+0.18%) ⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@12rambau 12rambau marked this pull request as ready for review November 28, 2022 21:36
Copy link
Collaborator

@dfguerrerom dfguerrerom left a comment

Choose a reason for hiding this comment

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

:)

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.

2 participants