Remove warnings in unit tests #509

danielhollas · 2023-09-03T23:39:11Z

Fixes #490.

TODO: Test things

I extracted some fixes that were actual bugs into separate PRs, #512 and #513. ~~This is blocked until they are merged~~ Now unblocked

codecov · 2023-09-03T23:43:14Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ccfcf80) 79.97% compared to head (dcdc755) 79.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
- Coverage   79.97%   79.92%   -0.06%     
==========================================
  Files          27       27              
  Lines        3830     3815      -15     
==========================================
- Hits         3063     3049      -14     
+ Misses        767      766       -1

Flag	Coverage Δ
python-3.10	`79.92% <96.55%> (-0.06%)`	⬇️
python-3.8	`79.95% <96.55%> (-0.06%)`	⬇️
python-3.9	`79.95% <96.55%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
aiidalab_widgets_base/data/__init__.py	`43.33% <ø> (ø)`
aiidalab_widgets_base/nodes.py	`95.20% <100.00%> (ø)`
aiidalab_widgets_base/structures.py	`77.92% <100.00%> (-0.07%)`	⬇️
aiidalab_widgets_base/viewers.py	`70.90% <100.00%> (-0.04%)`	⬇️
tests/test_databases.py	`100.00% <100.00%> (ø)`
tests/test_process.py	`95.68% <100.00%> (-0.22%)`	⬇️
tests/test_viewers.py	`100.00% <100.00%> (ø)`
aiidalab_widgets_base/process.py	`78.58% <0.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for more information, see https://pre-commit.ci

danielhollas

@yakutovicha @unkcpz I still need to test that I didn't break anything, but otherwise this is ready for review.

aiidalab_widgets_base/process.py

danielhollas · 2023-09-25T14:43:27Z

pyproject.toml

+
+[tool.pytest.ini_options]
+filterwarnings = [
+    'error',


Some of the warnings that I cleaned up in this PR uncovered actual bugs so imo it's good to just turn all warnings into errors so that we keep it clean. If it turned out to be too much chore we can always revert this decision.

It is a bit unfortunate that we need so many 'ignore's here but hopefully we'll reduce this as we update our third-party dependencies.

danielhollas · 2023-09-25T14:46:53Z

tests/test_process.py

@@ -218,26 +211,22 @@ def test_running_calcjob_output_widget(generate_calc_job_node):
        }
    )

-    # Test the widget can be instantiated with a process
-    RunningCalcJobOutputWidget(calculation=process)
+    widget = RunningCalcJobOutputWidget()


The RunninngCalcJobOutputWidget initializer does not have calculation as its input. This code was likely copy-pasted. This API is a bit unconsistent, since many other widgets allow passing the traitlet via constructor. But that's for another PR.

danielhollas · 2023-09-25T14:51:27Z

aiidalab_widgets_base/viewers.py

@@ -34,32 +34,22 @@ def registration_decorator(widget):
    return registration_decorator


-def viewer(obj, downloadable=True, **kwargs):


We can't be passing this attribute to the viewers in general because most of them do not have it, and then pass it up to the ipywidget parent which results in warnings. Since this parameter is only used in DictViewer and FolderDataViewer I think it is okay to get rid of it, but please correct me if I am wrong.

The problem maybe the viewer return from calling viewer directly will not be exactly the same as DictViewer. Where the downloadable can not pass to the viewer anymore.
I think it is fine to get rid of it but for the DictViewer the downloadable need to get from kwargs.

Sorry, I am not sure I got your comment. Do you want me to change anything?

The problem maybe the viewer return from calling viewer directly will not be exactly the same as DictViewer. Where the downloadable can not pass to the viewer anymore.

Yes. But other viewers may have other input parameters and we don't support them either, so I don't see why Downloadable should be privileged in any way.

Sorry, I am not sure I got your comment. Do you want me to change anything?

Never mind, I misunderstood the usage of the viewer in the first place. I was asking for changing __init__ of DictViewer, instead of passing downloadable but passing downloadable through kwargs.

It is not necessary, and your change is all good. The expected way of using the viewer function in the AiiDAlab is by AiidaNodeViewecWidget, which only passes the orm.Node without any parameters.

danielhollas · 2023-09-25T14:52:54Z

aiidalab_widgets_base/viewers.py

-    except KeyError as exc:
-        if obj.node_type in str(exc):
-            warnings.warn(
-                f"Did not find an appropriate viewer for the {type(obj)} object. Returning the object "


I don't find this warning particularly helpful since it is kinda self-evident. Also for Base types like Int it's perfectly okay not to have a dedicated viewer.

danielhollas · 2023-10-04T12:24:24Z

@unkcpz can you take a look? Would be nice to get this to 2.1 version so we have more confidence in our test suite.

unkcpz · 2023-10-04T13:25:34Z

Sure, I'll take it a look ASAP. In case you get more grumpy ;)

danielhollas · 2023-10-04T13:39:56Z

Sure, I'll take it a look ASAP. In case you get more grumpy ;)

Haha 🥲 That when I tried (ultimately unsuccessfully) to get rid of some warnings coming from Selenium.

unkcpz

Thanks @danielhollas! some minor things to discuss.

aiidalab_widgets_base/data/__init__.py

aiidalab_widgets_base/process.py

unkcpz · 2023-10-05T17:27:55Z

aiidalab_widgets_base/structures.py

@@ -658,8 +660,8 @@ def search(self, _=None):
        matches = {n[0] for n in qbuild.iterall()}
        matches = sorted(matches, reverse=True, key=lambda n: n.ctime)

-        options = OrderedDict()
-        options[f"Select a Structure ({len(matches)} found)"] = False
+        options = []


Since ipywidget use tuple as default for opitions (see https://github.com/jupyter-widgets/ipywidgets/blob/52663ac472c38ba12575dfb4979fa2d250e79bc3/python/ipywidgets/ipywidgets/widgets/widget_selection.py#L156-L166)

See the comment I just added, I think it is better to initiate the options as a tuple.

Here the options are generated dynamically from the search result and I do not see an easy way of using a tuple. Keeping a list here is fine imo.

unkcpz · 2023-10-05T17:35:53Z

aiidalab_widgets_base/viewers.py

@@ -34,32 +34,22 @@ def registration_decorator(widget):
    return registration_decorator


-def viewer(obj, downloadable=True, **kwargs):


The problem maybe the viewer return from calling viewer directly will not be exactly the same as DictViewer. Where the downloadable can not pass to the viewer anymore.
I think it is fine to get rid of it but for the DictViewer the downloadable need to get from kwargs.

danielhollas · 2023-10-12T00:35:11Z

@unkcpz the code is good to go from my side. However, I have not yet manually tested that I did not break anything (although we have better and better tests). If you could help with that that would be appreciated. The main thing is to test the viewer() since that has seen to most change.

unkcpz · 2023-10-12T08:54:17Z

I gave it a test on the QE app and it didn't break anything yet.
I'll make an beta release for AWB and do a thorough test on the rc release in QEapp.

unkcpz

Thanks! Looks all good to me. If you have no objection, I'll merge this and give a more detail test in the app.

danielhollas · 2023-10-12T12:30:43Z

Cool, let's go for it, I will also test it in my app. The other PRs should merge the master now to ensure that no new warnings are introduced. CC @yakutovicha

danielhollas force-pushed the fix/deprecation-warnings-misc branch from 131b49f to 6ed3e6f Compare September 6, 2023 08:39

danielhollas added 2 commits September 6, 2023 10:50

Remove warnings in unit tests

dee6b62

Update precommit action

106c913

danielhollas force-pushed the fix/deprecation-warnings-misc branch from 6ed3e6f to 106c913 Compare September 6, 2023 10:51

danielhollas added 7 commits September 6, 2023 16:08

Ignore remaining warnings, turn warnings into errors

e29dd56

Ignore DeprecationWarning from jupyter_core

2d7daab

Now?

b6ee99c

wtf

ff053d4

jupyter_client

0a10d8c

One more?

9ed6fbc

ignore selenium deprecationwarnings

1f75cb8

This was referenced Sep 7, 2023

Fix Warning coming from ProcessFollowerWidget #512

Merged

Fix .ssh/config file not being closed #513

Merged

danielhollas added the blocked This issue/PR is blocked by another issue/PR. label Sep 7, 2023

danielhollas and others added 7 commits September 7, 2023 18:23

Update selenium

e655614

Try selenium 4.8

c3f0c5b

Merge branch 'master' into fix/deprecation-warnings-misc

1644d8f

downgrade selenium back to 4.7

3bcead9

Do not error on warnings

b98285d

ignore pytest_selenium deprecation warnings

144f24f

error on warnings

7896eeb

danielhollas removed the blocked This issue/PR is blocked by another issue/PR. label Sep 25, 2023

danielhollas self-assigned this Sep 25, 2023

Merge branch 'master' into fix/deprecation-warnings-misc

1167ab6

danielhollas marked this pull request as ready for review September 25, 2023 08:45

pre-commit-ci bot and others added 3 commits September 25, 2023 08:45

[pre-commit.ci] auto fixes from pre-commit.com hooks

339dd61

for more information, see https://pre-commit.ci

ignore deprecationwarning from optimade_clien

fc58864

Fix ignore

fecf535

danielhollas commented Sep 25, 2023

View reviewed changes

danielhollas requested review from yakutovicha and unkcpz September 25, 2023 14:53

Cleanup

0bd3b09

Merge branch 'master' into fix/deprecation-warnings-misc

20e809b

unkcpz added this to the v2.1.0 milestone Oct 4, 2023

Merge branch 'master' into fix/deprecation-warnings-misc

2bf2ebd

unkcpz requested changes Oct 5, 2023

View reviewed changes

unkcpz mentioned this pull request Oct 9, 2023

Template resources setting #511

Merged

26 tasks

danielhollas added 2 commits October 12, 2023 01:16

review

9470aff

tuples all the way

dcdc755

danielhollas requested a review from unkcpz October 12, 2023 00:33

unkcpz approved these changes Oct 12, 2023

View reviewed changes

danielhollas merged commit ebc7120 into aiidalab:master Oct 12, 2023

danielhollas deleted the fix/deprecation-warnings-misc branch October 12, 2023 12:19

unkcpz mentioned this pull request Oct 12, 2023

Implement viewer representations #373

Merged

unkcpz mentioned this pull request Oct 20, 2023

Fix/remove deprecations aiidalab/aiidalab-qe#519

Draft

unkcpz added a commit to unkcpz/aiidalab-widgets-base that referenced this pull request Nov 16, 2023

The default value of workdir clean button set to False (aiidalab#509)

b9b86bb

unkcpz mentioned this pull request Jan 31, 2024

Get rid of ResourceWarning of pytest #551

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove warnings in unit tests #509

Remove warnings in unit tests #509

danielhollas commented Sep 3, 2023 •

edited

Loading

codecov bot commented Sep 3, 2023 •

edited

Loading

danielhollas left a comment

danielhollas Sep 25, 2023

danielhollas Sep 25, 2023

danielhollas Sep 25, 2023

unkcpz Oct 5, 2023

danielhollas Oct 11, 2023

unkcpz Oct 12, 2023

danielhollas Sep 25, 2023

danielhollas commented Oct 4, 2023

unkcpz commented Oct 4, 2023

danielhollas commented Oct 4, 2023

unkcpz left a comment

unkcpz Oct 5, 2023

unkcpz Oct 6, 2023

danielhollas Oct 12, 2023

unkcpz Oct 5, 2023

danielhollas commented Oct 12, 2023 •

edited

Loading

unkcpz commented Oct 12, 2023

unkcpz left a comment •

edited

Loading

danielhollas commented Oct 12, 2023

		@@ -34,32 +34,22 @@ def registration_decorator(widget):
		return registration_decorator


		def viewer(obj, downloadable=True, **kwargs):

Remove warnings in unit tests #509

Remove warnings in unit tests #509

Conversation

danielhollas commented Sep 3, 2023 • edited Loading

codecov bot commented Sep 3, 2023 • edited Loading

Codecov Report

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Oct 4, 2023

unkcpz commented Oct 4, 2023

danielhollas commented Oct 4, 2023

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Oct 12, 2023 • edited Loading

unkcpz commented Oct 12, 2023

unkcpz left a comment • edited Loading

Choose a reason for hiding this comment

danielhollas commented Oct 12, 2023

danielhollas commented Sep 3, 2023 •

edited

Loading

codecov bot commented Sep 3, 2023 •

edited

Loading

danielhollas commented Oct 12, 2023 •

edited

Loading

unkcpz left a comment •

edited

Loading