ec-jrc / lisflood-utilities

LISFLOOD OS - Utilities
https://ec-jrc.github.io/lisflood
European Union Public License 1.2
22 stars 9 forks source link

test_compare #22

Closed corentincarton closed 3 years ago

corentincarton commented 3 years ago

Hi,

I'm creating a testing environment at ECWMF which will test all the lisflood repos and how they interact together. I noticed that the unit test were not passing for lisflood-utilities. I get the following error:

tests/test_compare.py::TestComparators::test_pcr FAILED [ 41%]

=================================== FAILURES ===================================

___` TestComparators.test_pcr ___

self = <tests.test_compare.TestComparators object at 0x7fcd85786780>

def test_pcr(self):
    comp = PCRComparator(for_testing=True)
    with pytest.raises(AssertionError) as excinfo:
        comp.compare_files('tests/data/folder_b/1.map', 'tests/data/folder_c/1.map')
    assert 'tests/data/folder_b/1.map different from tests/data/folder_c/1.map' in str(excinfo.value)
    comp.compare_files('tests/data/folder_a/1.map', 'tests/data/folder_c/1.map')
    with pytest.raises(AssertionError) as excinfo:
        comp.compare_dirs('tests/data/folder_a/', 'tests/data/folder_b/', skip_missing=True)

  assert 'tests/data/folder_a/1.map different from tests/data/folder_b/1.map' in str(excinfo.value)

E AssertionError: assert 'tests/data/folder_a/1.map different from tests/data/folder_b/1.map' in 'tests/data/folder_a/4.map different from tests/data/folder_b/4.map' E + where 'tests/data/folder_a/4.map different from tests/data/folder_b/4.map' = str(AssertionError('tests/data/folder_a/4.map different from tests/data/folder_b/4.map',)) E + where AssertionError('tests/data/folder_a/4.map different from tests/data/folder_b/4.map',) = .value

comp = <lisfloodutilities.compare.pcr.PCRComparator object at 0x7fcd85786cc0> excinfo = self = <tests.test_compare.TestComparators object at 0x7fcd85786780>

I could fix it by changing the assertion on line 154 from assert 'tests/data/folder_a/1.map different from tests/data/folder_b/1.map' in str(excinfo.value) to assert 'tests/data/folder_a/4.map different from tests/data/folder_b/4.map' in str(excinfo.value)

but I don't have to necessary rights on this repo to create a pull request. Would it be possible to add me as developer for lisflood-utilities and for lisflood-calibration so that I can directly create pull requests in the future?

Cheers, Corentin

domeniconappo commented 3 years ago

Hi @corentincarton unfortunately, we ran out of external seats for repositories. We will discuss how to address this.

About the failing test, it was my error in writing it, because it depends on the dev machine OS (the test fails as soon as it finds a difference, so on some systems it checks 1.map first but it's not true in general. We need to make it independent from this kind of things. I'm going to fix it. Meanwhile, we'll try to find a solution to add more collaborators.

Thanks!

corentincarton commented 3 years ago

Hi @domeniconappo,

No worries, I just asked because it's sometimes easier to communicate suggestions using pull requests but there's no rush. Keep me posted when you push the fix.

Cheers, Corentin

domeniconappo commented 3 years ago

@corentincarton fixed

let me know if you still experience problems

corentincarton commented 3 years ago

Hi @domeniconappo,

test_compare seems to be fixed but now I get the following errors for test_cutmaps: ERROR: clone map 'tests/data/cutmaps/ldd_europe.map' does not exist I guess some test data files are missing... Closest thing I could only find in this folder was ldd_eu.nc.

domeniconappo commented 3 years ago

@corentincarton sorry..I forgot to add that file to repository.

Now it should be fine

corentincarton commented 3 years ago

@domeniconappo, thanks it's working fine now and all the tests are passing.

I don't know if you were there when we discussed that but I would suggest trying out the "Actions" feature of GitHub. For instance you could set it up to run the unit tests when someone is trying to merge a branch to master. That would ensure the stability of the master branch. It's quite straightforward to use but it may be behind a paywall. I'm just dropping this here because we have something similar in place at ECMWF on our bitbucket and it really helps for the code maintenance :)

Cheers, Corentin

domeniconappo commented 3 years ago

@corentincarton yes, it will help for sure.

I'm going to try to setup an action for this repo...