Unpin the `click` dependency #3921

sphuber · 2020-04-11T13:00:24Z

The breaking of the tests was caused by two separate commits:

718485be48263056e7036ea9a60ce11b47e2fc26
37d897069f58f7f2a016c14b0620c6d387430b4b

The first one changes the format of the output of a command if a
required option is missing. The double quotes around the option have
been changed to single quotes.

The second commit is more pernicious: the Editor.edit_file method was
updated to escape the editor and filename parameters passed to the sub
process call. Our tests were abusing the absence of escaping to pass
arguments to the editor command, in our case vim, as to make the
normally interactive command a non-interactive one for testing purposes.
Now that the editor command is escaped, the arguments are understood by
bash to be part of the command which of course then cannot be found and
the test fails. We "fix" this problem by patching the changed method in
click to undo the escaping.

codecov · 2020-04-11T15:17:20Z

Codecov Report

Merging #3921 into develop will increase coverage by 0.00%.
The diff coverage is 94.73%.

@@           Coverage Diff            @@
##           develop    #3921   +/-   ##
========================================
  Coverage    78.20%   78.21%           
========================================
  Files          460      460           
  Lines        34027    34035    +8     
========================================
+ Hits         26611    26619    +8     
  Misses        7416     7416

Flag	Coverage Δ
#django	`70.24% <94.73%> (+<0.01%)`	⬆️
#sqlalchemy	`71.10% <94.73%> (+<0.01%)`	⬆️

Impacted Files	Coverage Δ
aiida/manage/tests/pytest_fixtures.py	`89.47% <94.73%> (+1.71%)`	⬆️
aiida/transports/plugins/local.py	`80.20% <0.00%> (-0.26%)`	⬇️
aiida/cmdline/commands/cmd_code.py	`91.16% <0.00%> (+0.46%)`	⬆️

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 6ea9ecb...76ed3f6. Read the comment docs.

sphuber · 2020-04-11T15:28:15Z

/update-requirements

aiida-bot-app · 2020-04-11T15:28:17Z

Starting 'update-requirements' workflow for this branch.

sphuber · 2020-04-12T17:24:30Z

@ltalirz @greschd and @csadorf note that in the first commit I fix the version of click to be equal higher 7.1. This is just temporarily to force the tests to run on that version, since they were broken. After review, when you think this is good to be merged, I can revert the requirement to click~=7.0 since it should now be compatible with both minor versions.

ltalirz

thanks @sphuber !

I have a few comments & questions

tests/cmdline/commands/test_code.py

ltalirz · 2020-04-12T18:58:09Z

tests/cmdline/commands/test_code.py

+    assert isinstance(Code.get_from_string('{}@{}'.format(label, aiida_localhost.name)), Code)
+
+
+@pytest.mark.skip('This test is hanging for some reason.')


Can this be fixed?

I tried fixing it but I had even trouble understanding what the problem was. The problem was that the test hangs and you get no output. By changing the click source and setting default of resilient_parsing=False for the Context at least it printed a warning. Apparently the CodeBuilder was flagging an issue saying that properties were specified for a "remote" code even though it thinks that it concerns a local one. This is weird, because in the test what we are duplicating is a "remote" code. Anyway, I find the code of the CodeBuilder a bit hard to follow and I could not figure out why now all of a sudden it was complaining when I am just switching to a different manner of running the test and the test itself remaining essentially unchanged. Would be great if you may have some pointers, since I think you are the original author of the CodeBuilder.

since I think you are the original author of the CodeBuilder

I think this honor belongs to Rico, in 6873af9 .
However perhaps the issue is indeed in the lines I added for code duplication?
E.g. that the Code.get_code_spec function forgets to set some property?

aiida-core/aiida/orm/utils/builders/code.py

Lines 88 to 115 in 0644e7d

@staticmethod

def get_code_spec(code):

"""Get code attributes from existing code instance.

These attributes can be used to create a new CodeBuilder::

spec = CodeBuilder.get_code_spec(old_code)

builder = CodeBuilder(**spec)

new_code = builder.new()

"""

spec = {}

spec['label'] = code.label

spec['description'] = code.description

spec['input_plugin'] = code.get_input_plugin_name()

spec['prepend_text'] = code.get_prepend_text()

spec['append_text'] = code.get_append_text()

if code.is_local():

spec['code_type'] = CodeBuilder.CodeType.STORE_AND_UPLOAD

spec['code_folder'] = code.get_code_folder()

spec['code_rel_path'] = code.get_code_rel_path()

else:

spec['code_type'] = CodeBuilder.CodeType.ON_COMPUTER

spec['computer'] = code.get_remote_computer()

spec['remote_abs_path'] = code.get_remote_exec_path()

return spec

Anyhow, since Codes are now Nodes, perhaps this function is basically obsolete and one could simply clone the original Code instead?

Replacing this with a clone is probably still a valid point, perhaps open an issue for this if you agree?

I was going to, but it does quite a bit more then just providing the functionality that clone does. It also provides all the validation and a way for the contextual defaults to be set in the CLI, so I am not sure that we can actually get rid of it.

tests/cmdline/params/options/test_conditional.py

tests/conftest.py

sphuber · 2020-04-13T18:25:36Z

@ltalirz I fixed the hanging test and in doing so tackled another issue #3810 . If you are happy with the changes let me know, then I will rebase and revert the requirements to click~=7.0. It would maybe be good to also merge your PR #3894 before this, as it touches the test that was hanging and I now fixed.

ltalirz

thanks @sphuber - I have some minor comments.
Anyhow, you can also proceed and merge this as is.

aiida/manage/tests/pytest_fixtures.py

tests/cmdline/commands/test_code.py

tests/conftest.py

This was pinned because `7.1` broke our pre-commit hooks. The breaking of the tests was caused by two separate commits: * 718485be48263056e7036ea9a60ce11b47e2fc26 * 37d897069f58f7f2a016c14b0620c6d387430b4b The first one changes the format of the output of a command if a required option is missing. The double quotes around the option have been changed to single quotes. The second commit is more pernicious: the `Editor.edit_file` method was updated to escape the editor and filename parameters passed to the sub process call. Our tests were abusing the absence of escaping to pass arguments to the editor command, in our case `vim`, as to make the normally interactive command a non-interactive one for testing purposes. Now that the editor command is escaped, the arguments are understood by bash to be part of the command which of course then cannot be found and the test fails. We "fix" this problem by patching the changed method in `click` to undo the escaping.

sphuber requested a review from csadorf April 11, 2020 13:00

sphuber force-pushed the fix/3838/click-dependency branch from 2942ab2 to 039b7bb Compare April 11, 2020 15:02

sphuber force-pushed the fix/3838/click-dependency branch 2 times, most recently from e690333 to 3a7c2e6 Compare April 12, 2020 10:24

sphuber requested a review from greschd April 12, 2020 10:24

sphuber force-pushed the fix/3838/click-dependency branch 2 times, most recently from bf031a5 to cc7c0a0 Compare April 12, 2020 12:39

sphuber requested a review from ltalirz April 12, 2020 17:22

ltalirz reviewed Apr 12, 2020

View reviewed changes

sphuber force-pushed the fix/3838/click-dependency branch from cc7c0a0 to c46e55a Compare April 13, 2020 18:23

sphuber requested a review from ltalirz April 13, 2020 18:24

ltalirz approved these changes Apr 13, 2020

View reviewed changes

aiida/manage/tests/pytest_fixtures.py Outdated Show resolved Hide resolved

tests/cmdline/commands/test_code.py Outdated Show resolved Hide resolved

tests/conftest.py Outdated Show resolved Hide resolved

sphuber force-pushed the fix/3838/click-dependency branch from c46e55a to 9131f0b Compare April 13, 2020 20:40

sphuber force-pushed the fix/3838/click-dependency branch from 9131f0b to 76ed3f6 Compare April 13, 2020 21:18

sphuber merged commit 39d8ea0 into aiidateam:develop Apr 13, 2020

sphuber deleted the fix/3838/click-dependency branch April 13, 2020 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unpin the `click` dependency #3921

Unpin the `click` dependency #3921

sphuber commented Apr 11, 2020 •

edited

Loading

codecov bot commented Apr 11, 2020 •

edited

Loading

sphuber commented Apr 11, 2020

aiida-bot-app bot commented Apr 11, 2020

sphuber commented Apr 12, 2020

ltalirz left a comment

ltalirz Apr 12, 2020

sphuber Apr 12, 2020

ltalirz Apr 13, 2020 •

edited

Loading

ltalirz Apr 13, 2020

sphuber Apr 14, 2020

sphuber commented Apr 13, 2020

ltalirz left a comment

		assert isinstance(Code.get_from_string('{}@{}'.format(label, aiida_localhost.name)), Code)


		@pytest.mark.skip('This test is hanging for some reason.')

	@staticmethod
	def get_code_spec(code):
	"""Get code attributes from existing code instance.

	These attributes can be used to create a new CodeBuilder::

	spec = CodeBuilder.get_code_spec(old_code)
	builder = CodeBuilder(**spec)
	new_code = builder.new()

	"""
	spec = {}
	spec['label'] = code.label
	spec['description'] = code.description
	spec['input_plugin'] = code.get_input_plugin_name()
	spec['prepend_text'] = code.get_prepend_text()
	spec['append_text'] = code.get_append_text()

	if code.is_local():
	spec['code_type'] = CodeBuilder.CodeType.STORE_AND_UPLOAD
	spec['code_folder'] = code.get_code_folder()
	spec['code_rel_path'] = code.get_code_rel_path()
	else:
	spec['code_type'] = CodeBuilder.CodeType.ON_COMPUTER
	spec['computer'] = code.get_remote_computer()
	spec['remote_abs_path'] = code.get_remote_exec_path()

	return spec

Unpin the click dependency #3921

Unpin the click dependency #3921

Conversation

sphuber commented Apr 11, 2020 • edited Loading

codecov bot commented Apr 11, 2020 • edited Loading

Codecov Report

sphuber commented Apr 11, 2020

aiida-bot-app bot commented Apr 11, 2020

sphuber commented Apr 12, 2020

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Apr 12, 2020

Choose a reason for hiding this comment

sphuber Apr 12, 2020

Choose a reason for hiding this comment

ltalirz Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

ltalirz Apr 13, 2020

Choose a reason for hiding this comment

sphuber Apr 14, 2020

Choose a reason for hiding this comment

sphuber commented Apr 13, 2020

ltalirz left a comment

Choose a reason for hiding this comment

Unpin the `click` dependency #3921

Unpin the `click` dependency #3921

sphuber commented Apr 11, 2020 •

edited

Loading

codecov bot commented Apr 11, 2020 •

edited

Loading

ltalirz Apr 13, 2020 •

edited

Loading