search

aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
6 stars 17 forks source link

Add timeout to cod query test to avoid test hang #536

Closed unkcpz closed 8 months ago

unkcpz commented 8 months ago

Temporary solution to timeout the cod query widget test.

danielhollas commented 8 months ago

Hmm, I would think that the timeout needs to be placed after the widget is initialized inside the test?

unkcpz commented 8 months ago

I want to timeout the test itself to not block the other tests, although it will failed in timeout which is expected for this case, after the cod is back, the test will pass.

danielhollas commented 8 months ago

I don't understand. Why is it expected that the test should timeout? We ideally want to fix the test itself no? Surely when the test times out it should fail the test suite, even if all other tests pass.

unkcpz commented 8 months ago

Why is it expected that the test should timeout?

I don't think we can "fix" it, the problem is for the cod server side. Once the server is working, the test will pass. So if we find only this test failed and the cod server is not accessible, we can still continue with merging the PRs.

danielhollas commented 8 months ago

the problem is for the cod server side. Once the server is working,

Okay, could you please open an issue and explain in detail what the problem is? In the meantime, I would suggest to simply skip the test (@pytest.mark.skip I think). That's a much easier and more robust solution than the timeout.

yakutovicha commented 8 months ago

The test is passing now, btw.

unkcpz commented 8 months ago

@yakutovicha thanks! I think it is not a bad idea to add the timeout, then we don't stuck the test in the future if the cod server not accessible again in the future (although quite rare I hope). If skip it, then it requires to change the test by PRs. If you think it not worth to do like this I'll close the PR.

yakutovicha commented 8 months ago

I think it is not a bad idea to add the timeout, then we don't stuck the test in the future if the cod server not accessible again in the future (although quite rare I hope).

So as far as I can see, adding the timeout doesn't solve the test problem itself, but just speeds up the test failure in case of trouble. Which is not a bad thing, in the end.

To resolve the issue we might monkey-patch the output of the request sent to the COD, but that is for another PR. Not sure if this is a good idea, though. Because if COD changes something, then the problem might go unnoticed.

Another (somewhat radical) idea is to deprecate the widget and only rely on the OPTIMADE one, which also includes COD.

unkcpz commented 8 months ago

@danielhollas, do you agree with this change?

danielhollas commented 8 months ago

Sure, let's try it, but the tests are currently failing??

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (9277a67) 85.82% compared to head (3600ab3) 85.83%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #536 +/- ## ======================================= Coverage 85.82% 85.83% ======================================= Files 27 27 Lines 4608 4610 +2 ======================================= + Hits 3955 3957 +2 Misses 653 653 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/536/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/536/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `85.83% <100.00%> (+<0.01%)` | :arrow_up: | | [python-3.8](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/536/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `85.86% <100.00%> (+<0.01%)` | :arrow_up: | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/536/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `85.86% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/536?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [tests/test\_databases.py](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/536?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#diff-dGVzdHMvdGVzdF9kYXRhYmFzZXMucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

unkcpz commented 8 months ago

I didn't retrigger the test after COD was alive. All passed now :)