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.

Sign up for GitHub

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

Jump to bottom

Make LocalNode and RemoteNode extendable classes (yml-wise) #1280

Closed
wants to merge 3 commits into from
Closed

Make LocalNode and RemoteNode extendable classes (yml-wise) #1280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb4f983
✨ Make LocalNode and RemoteNode extendable classes (yml-wise).
glima Apr 14, 2021
b0bc822
poetry: update
glima Apr 14, 2021
d61780e
🔊 [wait_tcp_port_ready] make logs more meaningful, by showing name of…
glima Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion examples/testsuites/withscript.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from lisa import Node, TestCaseMetadata, TestSuite, TestSuiteMetadata
from lisa.executable import CustomScript, CustomScriptBuilder
from lisa.node import is_remote
from lisa.operating_system import Windows
from lisa.testsuite import simple_requirement
from lisa.util.perf_timer import create_timer
Expand Down Expand Up @@ -44,7 +45,7 @@ def script(self, node: Node) -> None:
timer2 = create_timer()
result2 = script.run(force_run=True)
assert_that(result1.stdout).is_equal_to(result2.stdout)
if node.is_remote:
if is_remote(node):
# the timer will be significant different on a remote node.
assert_that(
timer1.elapsed(), "the second time should be faster, without uploading"
Expand Down
18 changes: 12 additions & 6 deletions lisa/executable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from hashlib import sha256
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Type, TypeVar, Union, cast

from lisa.schema import RemoteNode
from lisa.util import InitializableMixin, LisaException, constants
from lisa.util.logger import get_logger
from lisa.util.perf_timer import create_timer
Expand All @@ -20,6 +21,11 @@
T = TypeVar("T")


# circular dep if we use the helper in node.py, unfortunately
def _is_node_remote(node: Node) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

How about define is_remote to a member of Node? It will not be cycle reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not have a method on the parent about children

Copy link
Member

Choose a reason for hiding this comment

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

See my PR, it can be an abstract method, no type checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still ugly to infer anything about extensions on a base class, that breaks OO totally :/

Copy link
Member

Choose a reason for hiding this comment

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

is_remote is a feature on node like is_posfix. Some case cannot run locally, like smoke_test. It's high cost to reboot current machine, and resume tests.

Copy link
Contributor Author

@glima glima Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I know, but it belongs in the module, not the class. OS is one class only, node is not anymore. The module knows there is more to a Node, the class should not have anything to do with it.

return isinstance(node.type_schema, RemoteNode)
squirrelsc marked this conversation as resolved.
Show resolved Hide resolved


class Tool(ABC, InitializableMixin):
"""
The base class, which wraps an executable, package, or scripts on a node.
Expand Down Expand Up @@ -246,10 +252,10 @@ def run(

def get_tool_path(self) -> pathlib.PurePath:
"""
compose a path, if the tool need to be installed
compose a path, if the tool needs to be installed
"""
assert self.node.remote_working_path, "remote working path is not initialized"
return self.node.remote_working_path.joinpath(constants.PATH_TOOL, self.name)
assert self.node.working_path, "working path is not initialized"
return self.node.working_path.joinpath(constants.PATH_TOOL, self.name)

def __call__(
self,
Expand Down Expand Up @@ -359,7 +365,7 @@ def dependencies(self) -> List[Type[Tool]]:
return self._dependencies

def install(self) -> bool:
if self.node.is_remote:
if _is_node_remote(self.node):
# copy to remote
node_script_path = self.get_tool_path()
for file in self._files:
Expand Down Expand Up @@ -495,15 +501,15 @@ def __getitem__(self, tool_type: Union[Type[T], CustomScriptBuilder, str]) -> T:
is_success = tool.install()
if not is_success:
raise LisaException(
f"install '{tool.name}' failed. After installed, "
f"installing '{tool.name}' has failed. After installed, "
f"it cannot be detected."
)
tool_log.debug(f"installed in {timer}")
else:
raise LisaException(
f"cannot find [{tool.name}] on [{self._node.name}], "
f"{self._node.os.__class__.__name__}, "
f"Remote({self._node.is_remote}) "
f"Remote({_is_node_remote(self._node)}) "
f"and installation of [{tool.name}] isn't enabled in lisa."
)
else:
Expand Down
Loading