Fixing anti-patterns by andyleejordan · Pull Request #1039 · microsoft/lisa

andyleejordan · 2020-10-02T23:46:43Z

This is a work-in-progress, but I am trying to fix anti-patterns as I find them (and then document best practices in the contributing guidelines) and clean up internal documentation etc. However, at this point I don't think this work will ever be finished.

As I got further into reviewing the prototype, I am unsure if this is what we want. There's a lot of code that's reimplementing a unit test framework (like collecting and running tests, e.g. all of the metadata decorator class logic), which is something many libraries already do (see unittest and pytest). There's also a lot of difficult dependencies already. For instance:

I can't rename test_lisarunner.py to test_runner.py because doing so breaks the tests. The global variables in test_platform.py end up as empty lists, breaking an assertion in the runner tests. This seems to be caused by an implicit alphabetical dependency based on the module loading sequence, but I couldn't fix it.
I can't turn LisaTestMetadata into a dataclass, because it's not actually fully defined as a class. It has self.requirement and self.suite defined at runtime. There are two big issues here creating a difficult-to-maintain codebase: the class abuses __getattr__ to pretend to inherit the LisaTestCaseMetadata class, by forwarding attributes to self.suite if they don't exist. Hence why self.requirement isn't optionally set, it's optionally defined as an attribute in the first place. There is also an issue with self.suite itself, where it's not defined except at runtime as the classes are instantiated through their decorator __call__ attribute (and at that, in a function of a function). This causes obvious typing errors, and instead of designing to avoid such an issue, dead code was introduced in the form of set_suite(self) which is never called.
As can be seen in some of my commits, I encountered a lot of extra abstraction that is unnecessary and makes the code obtuse, such as LisaRunner(Action) where the Action class does nothing of value, and the LisaRunner class really only matters for its start function. This same kind of pattern repeats itself throughout the code.
The base TestSuite class derives from unittest.TestCase, which implies that we actually are using unittest for all its value (such as discovering and running tests), but this is not the case. We're simply reusing its assertions and reimplementing everything else. It's very confusing. This also introduced mixed nomenclature: Since we're (kind of) reusing the unittest module’s TestCase class, we should not introduce confusing new nomenclature like TestSuite, since a "suite" is different from a "case" in usage.
I also found that one of the primary libraries used (spur) just leaks resources (never closes stdin file descriptors when it runs a process), which would lead to long-term issues. We simply shouldn't be using a library of such poor quality, but it's already deeply embedded in this code. In this PR I implemented a hacky fix for this problem, but it's not a solution. Moreover this lead to the discovery of our process control logic using a lot of duck typing (doing different things based on the type of a field, which is changing at runtime), which is a hard-to-maintain design pattern and violates the point of our static type checking.
Nothing internally is documented as to its intent and design, leaving a maintainer to just guess why things were done (which is what I've spent the week doing). It appears that a lot of design ideas were borrowed from Python's unittest module (based on name choices precisely matching their own modules), but since we're not leveraging unittest, I can't really say.

There's a lot more that I discovered, and frankly we are already heavily violating two of our main design goals:

LISAv3 uses modular design; easy to maintain and extend (Low maintenance cost).
LISAv2 global state/variable brings runtime troubles.

I'm considering going back to the drawing board, because I know that we need to not repeat the mistakes of LISAv2, and this must be maintainable. I have a tentative plan drafted to make LISAv3 a plugin for pytest, but am still working on it.

Update:

Here's a working demo using Pytest instead. (Note that my machine has SSH config setup to connect to a node via the name "centos", which won't work for you out of the box.)

The `-X dev` option reveals useful information such as leaked system resources.

Forgot this.

At minimum this converts the usage to the documented approach where this is exactly one asyncio event loop (started in main) and everything that currently is written async actually becomes a coroutine. The unit tests which use coroutines now use `IsolatedAsyncioTestCase` instead of just `TestCase`, which ensures they continue to work.

Instead of aliasing this function, we should just rename it entirely. Instead of delegating the parsing of an `argparse.Namespace` object, we just setup the `runbook` argument to be a `Path` instead of a string, and have `load_runbook` accept what it needs instead of digging it out of the `Namespace`.

And use single underscore to avoid invoking name mangling. If the name is reused in a subclass, we likely do not intend to have a duplicate variable, but are intending to use it. For variables we expect to be used, we should use Python’s built-in `property` class/decorator.

It was not being used for anything useful.

This was an abstract class that provided no value, just extra abstraction. Originally it was going to be used by multiple runner implementations, and the notifier, and for artifacts. However, we only have one runner which no longer uses it, the notifier never used it, and artifacts aren’t implemented so it’s too early to say if it’s useful there. Hence we should delete this to reduce complexity.

Since there is no longer a corresponding `stop` (and when there was a `stop` it didn’t actually do anything).

Since it’s really just a list comprehension. Also add a bunch of notes identifying things to investigate further.

These functions did not need a class, they can run be themselves. There’s some work to fix the functions which still rely entirely on side-effects, but this reduces a lot of complexity (as evidenced by the changes in the tests, half of which no longer generate a `Runner` instance at all).

Since it now always expects an `env_runbook` we can just construct it on-the-fly.

Because an implicit alphabetical dependency in the test module names breaks the tests.

There’s more work to do because this was using duck typing. It’s a little better now, but it’s just a massive wrapper for a library that’s just another wrapper for Python’s actual APIs (which are just wrappers for the system’s APIs).

Since we’re reusing the unittest module’s `TestCase` class, we should not introduce confusing new nomenclature like `TestSuite`, since a “suite” is different from a “case” in usage.

Also rename (and delete where appropriate) the respective functions (and start cleaning them up, though they need more work).

And start updating other functions.

`LisaTestCase` is _not_ an abstract base class, it’s just a base class (with a lot of implementation) and defines no abstract methods. So we remove this.

Again for consistency.

By using @DataClass. Also, this actually a case for multiple inheritance instead of abuse of `__getattr__`.

squirrelsc · 2020-10-12T03:36:07Z

@andschwa see comments inline. Thank you a lot! It's not only an improvement of code base, also give me a chance to explain "what's in my mind". I know I said it many times, but do nothing. I will prioritize my document work, it's definitely a tech debt now.
@lpereira @andschwa feel free to correct me, or provide better solutions.

This is a work-in-progress, but I am trying to fix anti-patterns as I find them (and then document best practices in the contributing guidelines) and clean up internal documentation etc. However, at this point I don't think this work will ever be finished.

As I got further into reviewing the prototype, I am unsure if this is what we want. There's a lot of code that's reimplementing a unit test framework (like collecting and running tests, e.g. all of the metadata decorator class logic), which is something many libraries already do (see unittest and pytest). There's also a lot of difficult dependencies already. For instance:

It uses internal unittest is for reusing all assertion, they are defined and implemented very well. You can consider it likes a mixin.
The collecting and running tests are very different with existing test frameworks.
The decorator is to force our test code guideline. For example, the description and priority must be set for each case. Requirement can be defined at suite level, so it doesn't need to repeat on case level.

I can't rename test_lisarunner.py to test_runner.py because doing so breaks the tests. The global variables in test_platform.py end up as empty lists, breaking an assertion in the runner tests. This seems to be caused by an implicit alphabetical dependency based on the module loading sequence, but I couldn't fix it.

It can be fixed like to use a test object for MockPlatform. File an issue or task to me.

I can't turn LisaTestMetadata into a dataclass, because it's not actually fully defined as a class. It has self.requirement and self.suite defined at runtime.

requirement can be optional, and set it when initializing suite and cases.

There are two big issues here creating a difficult-to-maintain codebase: the class abuses __getattr__ to pretend to inherit the LisaTestCaseMetadata class, by forwarding attributes to self.suite if they don't exist. Hence why self.requirement isn't optionally set, it's optionally defined as an attribute in the first place.

Not quite understand point here. The metadata of suite is the fallback for its cases, it can reuse shared settings like requirement. __getattr__ may not a good way to implement this purpose.

There is also an issue with self.suite itself, where it's not defined except at runtime as the classes are instantiated through their decorator __call__ attribute (and at that, in a function of a function).

__call__ in decorator is common way to customize a decorator. function is first class object in Python.

This causes obvious typing errors, and instead of designing to avoid such an issue, dead code was introduced in the form of set_suite(self) which is never called.

As my understand on goal/non-goal of static typing, it's a compromise. Typing can help improve productivity on development, but it looks wired sometime as Python is a dynamic language. In this case, we can add self.suite: TestSuiteMetadata in __init__ to remove set_suite. The set_suite may be a dead code, or I forgot to call it..

As can be seen in some of my commits, I encountered a lot of extra abstraction that is unnecessary and makes the code obtuse, such as LisaRunner(Action) where the Action class does nothing of value, and the LisaRunner class really only matters for its start function. This same kind of pattern repeats itself throughout the code.

We can remove Action for now, but may need to add it back later. I already implemented message in Action, but it's deleted in this PR. It's ok, we can add it back later. The Action uses to manage and monitor states of key components. For example, when we implementing UI like Cirrus, user can see sub-progress of a run by message. User can stop one of component from UI.

The base TestSuite class derives from unittest.TestCase, which implies that we actually are using unittest for all its value (such as discovering and running tests), but this is not the case. We're simply reusing its assertions and reimplementing everything else. It's very confusing.

You are right. The unittest uses to provide the asserts only. I tried to find another layer to reuse, but it's tightly coupled together. One mitigation can be, we create an alias like 'LisaAssertionMixin = Union[unittest.TestCase, Assertions]', and Assertions is a duck type, and include all assertions definition. It looks like what typing doing.

This also introduced mixed nomenclature: Since we're (kind of) reusing the unittest module’s TestCase class, we should not introduce confusing new nomenclature like TestSuite, since a "suite" is different from a "case" in usage.

the concepts of suite and case are what we have to redefine.

I also found that one of the primary libraries used (spur) just leaks resources (never closes stdin file descriptors when it runs a process), which would lead to long-term issues. We simply shouldn't be using a library of such poor quality, but it's already deeply embedded in this code. In this PR I implemented a hacky fix for this problem, but it's not a solution. Moreover this lead to the discovery of our process control logic using a lot of duck typing (doing different things based on the type of a field, which is changing at runtime), which is a hard-to-maintain design pattern and violates the point of our static type checking.

First, it's really a good fix! I saw it, and want to fix it recently. Well, I'm trying to avoid reinvent wheels, and it's reality of opensource world. You know a package deeply, after using it deeply. We may can contribute back some of fixes to mitigate it. Let me know, if there is better package or we have to reinvent it.

Nothing internally is documented as to its intent and design, leaving a maintainer to just guess why things were done (which is what I've spent the week doing).

I'm really sorry about that. I wrote a PPT and shared one week ago. hope it's helpful. We can schedule some meetings to help understand some purpose. I will prioritize some document works earlier.

It appears that a lot of design ideas were borrowed from Python's unittest module (based on name choices precisely matching their own modules), but since we're not leveraging unittest, I can't really say.

The design ideas are not from Python's unittest, it's common practice of test suite frameworks. The concept of test case is very different between lisa and python's unittest. The lisa has test suite, it includes a couple of test cases. Python's unittest.TestCase includes a couple of test methods. As lisa's practice, we need to define test purpose, priority and other information for each test case.

There's a lot more that I discovered, and frankly we are already heavily violating two of our main design goals:

LISAv3 uses modular design; easy to maintain and extend (Low maintenance cost).

I don't agree, if evidences are bullet points above. The bullet points prove it anti-pattern, it effects maintenance, but not heavily. All anti-patterns fixes are on framework, not about extension and test case development experience. Thank you a lot, I see many of them fixed! This goal means below to me,
- Centralize logic of platform, executable, test cases to share more logic, and improve maintenance capability.
  - The azure platform code is growing complex. With supports of the concept of Feature. There is no azure related code needed be outside of this folder. The LISAv2 is deeply couple with Azure vm_size, so that it's hard to understand what's real requirement of a test case, and the vm_size needs to be translate to HyperV and other platforms.
  - The executable and operating system supports to handle various difference of distros and operating system. If you see code of lscpu.py, you can see how different logic can be handled for different os in one central module. It can supports different distros with similar logic. If you see how modinfo used in LISAv2, you will know how it's important to make code easy to maintain.
  - Test case is fully decoupled with Azure, HyperV with requirement and capability. Test case developers just need to specify requirements of test cases, lisa will translate it to various information like vm_size, sriov and so on. It still needs onboard real test cases to verify, but I have confidence it's the right way.
- Extension by modules. We're implementing a notifier to save test results as our internal database schema. If there are internal tools, scripts, distros, they can be loaded by path, and test cases can use them when developing. It' provides both runtime capability and development experience.

LISAv2 global state/variable brings runtime troubles.

I don't agree. Firstly, we have different understanding on this goal. IMO, lisa v2 has a lot of built-in parameters, which pass through from command line and changed on somewhere by work arounds. There always variables need to be passed in, and may be changed by different purpose. This goal is not to elimate parameters, it's to make things sense, and easy to understand. If see your above troubles of global variables, all of them are in lisa test code. I know it's not good practice, and thank you to bring them out. Below is points how parameters handles better in lisa v3.
- Minimize built-in parameters. If you see argparser.py, lisa v3 implements very limited built-in parameters. Comparing it with how it's implemented in lisav2, you will see what's significant improved.
- Test variables are defined by users, not lisa. For example, vm_size must be a variable by chaos monkey testing as a variable, but xdp needs a fixed vm_size. It depends on user's test purpose to decide using variable or not.
- Limit parameter changes on specified locations. For example, if vm_size is set by a variable, it can be changed by preparing phase of azure platform. No other place can change it.
- Logging variable changes clearly. Use vm_size as an example again.
  1. After runbook loaded with all variables, it dumps in log. So that user can check if there is any misconfiguration in runbook.
  2. When preparing the environment, it may change vm_size there. The prepared vm_size is dumped here to understand any changes on vm_size.
  3. When vm_size passes into arm_template, it's dumped again. The vm_size won't be changed any more after it's passed into arm template. It's a final gatekeeper to help identifying any unexpected changes.

I'm considering going back to the drawing board, because I know that we need to not repeat the mistakes of LISAv2, and this must be maintainable. I have a tentative plan drafted to make LISAv3 a plugin for pytest, but am still working on it.

I really like what pytest can do, please go ahead to check if it can be a plug-in of pytest. At the same time, can we have pytest replace current unittest.TestCase? If so, test case developers can use new experience directly. We don't have many test cases today.

Update:

Here's a working demo using Pytest instead. (Note that my machine has SSH config setup to connect to a node via the name "centos", which won't work for you out of the box.)

squirrelsc · 2020-10-12T03:42:20Z

 * Always use `cls` for the first argument to class methods.
 * Use one leading underscore only for non-public methods and instance variables,
-  such as `_data`.
+  such as `_data`. Do not activate name mangling with `__` unless necessary.


as pep8, _name uses to mark "subclass API". __ is for private in this case.

squirrelsc · 2020-10-12T04:14:01Z

+* If there is a pair of `get_x` and `set_x` methods, they should instead be a
+  proper property, which is easy to do with the built-in `@property` decorator.
 * Constants should be `CAPITALIZED_SNAKE_CASE`.
+* When importing a function, try to avoid renaming it with `import as` because


It's my previous experience, and can be fixed and document. for example, load function in runbook, it should be imported like from lisa.parameter_parser import runbook, and used like runbook.load. So that it doesn't need as, and clear on using.

squirrelsc · 2020-10-12T07:33:16Z

 * Constants should be `CAPITALIZED_SNAKE_CASE`.
+* When importing a function, try to avoid renaming it with `import as` because
+  it introduces cognitive overhead to track yet another name.
+* When deriving another module’s class (such as `unittest.TestCase`), reuse the


As my explanation in overall comments, unittest.TestCase is a mixin actually. We can raise another case to explain this point.

squirrelsc · 2020-10-12T07:35:24Z



-@TestSuiteMetadata(
+@LisaTestCaseMetadata(


Keep class -> suite, and method -> case. See my explanation in overall comments, we have different concept on them than Python ut. We're not write UT, so the concept of suite and case are heavier than Python UT.

andyleejordan · 2020-10-12T17:12:17Z

Hi @squirrelsc, please take a look at this demo I constructed over the weekend. I intend to demo it and explain its design at today's sync meeting: #1044

andyleejordan · 2020-10-13T23:11:57Z

@andschwa see comments inline. Thank you a lot! It's not only an improvement of code base, also give me a chance to explain "what's in my mind". I know I said it many times, but do nothing. I will prioritize my document work, it's definitely a tech debt now.
@lpereira @andschwa feel free to correct me, or provide better solutions.

Glad you appreciated the review! I just wish we'd been able to do it before merging this code originally. It is so important for us to get the design right, I just don't think we've done that yet.

This is a work-in-progress, but I am trying to fix anti-patterns as I find them (and then document best practices in the contributing guidelines) and clean up internal documentation etc. However, at this point I don't think this work will ever be finished.

As I got further into reviewing the prototype, I am unsure if this is what we want. There's a lot of code that's reimplementing a unit test framework (like collecting and running tests, e.g. all of the metadata decorator class logic), which is something many libraries already do (see unittest and pytest). There's also a lot of difficult dependencies already. For instance:

It uses internal unittest is for reusing all assertion, they are defined and implemented very well. You can consider it likes a mixin.

I don't believe this is strictly true. As implemented, it appears many designs were borrowed from unittest (see the runner for instance), but reimplemented instead of reused. Also, we should not be pulling in something like this just to provide assertions, especially when there is already much critique of the way that unittest handles assertions.

The collecting and running tests are very different with existing test frameworks.

I disagree with this. The logic to collect and run tests is a solved problem for which we do not need to (nor do we want to) implement yet another test framework. Our logic is no different than any other test framework, it just has our own specifics. Can you elaborate in what our logic is so different that we have to abandon all other test frameworks?

The decorator is to force our test code guideline. For example, the description and priority must be set for each case. Requirement can be defined at suite level, so it doesn't need to repeat on case level.

That's fine, but it's not something we need to reimplement an entire framework to accomplish. It can be implemented in a simpler manner.

I can't rename test_lisarunner.py to test_runner.py because doing so breaks the tests. The global variables in test_platform.py end up as empty lists, breaking an assertion in the runner tests. This seems to be caused by an implicit alphabetical dependency based on the module loading sequence, but I couldn't fix it.

It can be fixed like to use a test object for MockPlatform. File an issue or task to me.

My point here is not that there is one bug, it's that there's so much tightly coupled code that it was impossible to change a simple thing (the name of a test file). This feels like a legacy codebase with the amount of tech debt that's already built up.

I can't turn LisaTestMetadata into a dataclass, because it's not actually fully defined as a class. It has self.requirement and self.suite defined at runtime.

requirement can be optional, and set it when initializing suite and cases.

That means it should be optionally set not optionally defined, which is my original point. The actual defined attributes of the class are changing at runtime, which will (and already does) lead to lots of maintenance headaches.

There are two big issues here creating a difficult-to-maintain codebase: the class abuses __getattr__ to pretend to inherit the LisaTestCaseMetadata class, by forwarding attributes to self.suite if they don't exist. Hence why self.requirement isn't optionally set, it's optionally defined as an attribute in the first place.

Not quite understand point here. The metadata of suite is the fallback for its cases, it can reuse shared settings like requirement. __getattr__ may not a good way to implement this purpose.

The point is the same as above. The class is not fully defined, ever. It's shape (what fields and types of those fields it has) changes at runtime. This is not defensive programming and defeats the point of us having any type checking to begin with.

There is also an issue with self.suite itself, where it's not defined except at runtime as the classes are instantiated through their decorator __call__ attribute (and at that, in a function of a function).

__call__ in decorator is common way to customize a decorator. function is first class object in Python.

I understand what a decorator is and how it works. Again, the point is the same as above, the class itself is not fully defined because the code is defining attributes (not setting their values) at runtime.

This causes obvious typing errors, and instead of designing to avoid such an issue, dead code was introduced in the form of set_suite(self) which is never called.

As my understand on goal/non-goal of static typing, it's a compromise. Typing can help improve productivity on development, but it looks wired sometime as Python is a dynamic language. In this case, we can add self.suite: TestSuiteMetadata in __init__ to remove set_suite. The set_suite may be a dead code, or I forgot to call it..

This again ties into the above point. The static type analysis is pointing out the inherent issue above: that the classes are not fully defined. Instead of fixing this and ensuring the classes were fully defined, an attempt was made to hide the type error by introducing this dead code. The function is never called, instead the type is defined and its value set for each instance of the class at runtime within three nested function calls from the point of the decorator being used.

As can be seen in some of my commits, I encountered a lot of extra abstraction that is unnecessary and makes the code obtuse, such as LisaRunner(Action) where the Action class does nothing of value, and the LisaRunner class really only matters for its start function. This same kind of pattern repeats itself throughout the code.

We can remove Action for now, but may need to add it back later. I already implemented message in Action, but it's deleted in this PR. It's ok, we can add it back later. The Action uses to manage and monitor states of key components. For example, when we implementing UI like Cirrus, user can see sub-progress of a run by message. User can stop one of component from UI.

I think the Action class is a prime example of over-abstracting and under-designing. There's no design document describing what or why we need this class, and what value it provides, and what all is going to subclass it.

The base TestSuite class derives from unittest.TestCase, which implies that we actually are using unittest for all its value (such as discovering and running tests), but this is not the case. We're simply reusing its assertions and reimplementing everything else. It's very confusing.

You are right. The unittest uses to provide the asserts only. I tried to find another layer to reuse, but it's tightly coupled together. One mitigation can be, we create an alias like 'LisaAssertionMixin = Union[unittest.TestCase, Assertions]', and Assertions is a duck type, and include all assertions definition. It looks like what typing doing.

This also introduced mixed nomenclature: Since we're (kind of) reusing the unittest module’s TestCase class, we should not introduce confusing new nomenclature like TestSuite, since a "suite" is different from a "case" in usage.

the concepts of suite and case are what we have to redefine.

If we are redefining them we should clearly document such, but I could not find that. Instead we are mixing nomenclatures, and not explaining our new one. I also see no reason to diverge from an existing an well known nomenclature that we have also brought into the codebase.

I also found that one of the primary libraries used (spur) just leaks resources (never closes stdin file descriptors when it runs a process), which would lead to long-term issues. We simply shouldn't be using a library of such poor quality, but it's already deeply embedded in this code. In this PR I implemented a hacky fix for this problem, but it's not a solution. Moreover this lead to the discovery of our process control logic using a lot of duck typing (doing different things based on the type of a field, which is changing at runtime), which is a hard-to-maintain design pattern and violates the point of our static type checking.

First, it's really a good fix! I saw it, and want to fix it recently. Well, I'm trying to avoid reinvent wheels, and it's reality of opensource world. You know a package deeply, after using it deeply. We may can contribute back some of fixes to mitigate it. Let me know, if there is better package or we have to reinvent it.

That is precisely why we are taking our time to review this code carefully and decide if it's what we want. After using it, we've discovered that we don't want Spur. It would have been nice to discover this sooner, but better late than never!

Nothing internally is documented as to its intent and design, leaving a maintainer to just guess why things were done (which is what I've spent the week doing).

I'm really sorry about that. I wrote a PPT and shared one week ago. hope it's helpful. We can schedule some meetings to help understand some purpose. I will prioritize some document works earlier.

Please don't rely on internally shared documents for the documentation of an open source project. While I have reviewed those slides, they did not help me understand the design decisions of the implementation that I reviewed here.

It appears that a lot of design ideas were borrowed from Python's unittest module (based on name choices precisely matching their own modules), but since we're not leveraging unittest, I can't really say.

The design ideas are not from Python's unittest, it's common practice of test suite frameworks. The concept of test case is very different between lisa and python's unittest. The lisa has test suite, it includes a couple of test cases. Python's unittest.TestCase includes a couple of test methods. As lisa's practice, we need to define test purpose, priority and other information for each test case.

I think you are realizing my own point here: these designs are common because test discovery, filtering, and running are solved problems. Let's not reinvent the wheel.

There's a lot more that I discovered, and frankly we are already heavily violating two of our main design goals:

LISAv3 uses modular design; easy to maintain and extend (Low maintenance cost).

I don't agree, if evidences are bullet points above. The bullet points prove it anti-pattern, it effects maintenance, but not heavily. All anti-patterns fixes are on framework, not about extension and test case development experience. Thank you a lot, I see many of them fixed! This goal means below to me,

I posit that this codebase as it stands is not maintainable. It's in about the same state as LISAv2. The framework is what's important for us to get right, because it's from the framework that everything else will be accomplished.

Centralize logic of platform, executable, test cases to share more logic, and improve maintenance capability.

Hm?

The azure platform code is growing complex. With supports of the concept of Feature. There is no azure related code needed be outside of this folder. The LISAv2 is deeply couple with Azure vm_size, so that it's hard to understand what's real requirement of a test case, and the vm_size needs to be translate to HyperV and other platforms.

That's fine, and I think a laudable goal we can accomplish, but it doesn't mean we need to use this code.

The executable and operating system supports to handle various difference of distros and operating system. If you see code of lscpu.py, you can see how different logic can be handled for different os in one central module. It can supports different distros with similar logic. If you see how modinfo used in LISAv2, you will know how it's important to make code easy to maintain.

I'm not sure I agree that this is good idea. What we're talking about here is a platform abstraction layer, which is separate from a test framework. Many (many, many) PALs have been written, and they're all encumbered and difficult to maintain. If we implement any part of a PAL, we should only do so when we have enough code (read: tests) doing the same thing that it would actually benefit us to refactor it, not just because we (like everyone before us) thought it was a good idea and wanted it. I think it will cost us more than it will benefit us and urge caution against writing yet another PAL.

Test case is fully decoupled with Azure, HyperV with requirement and capability. Test case developers just need to specify requirements of test cases, lisa will translate it to various information like vm_size, sriov and so on. It still needs onboard real test cases to verify, but I have confidence it's the right way.

Let's focus first on lifting-and-shifting the existing LISAv2 tests before we write a new requirements framework. We need the existing tests moved first in order to correctly reason about what a requirements/capability framework should look like, it's not something we should just guess at.

Extension by modules. We're implementing a notifier to save test results as our internal database schema. If there are internal tools, scripts, distros, they can be loaded by path, and test cases can use them when developing. It' provides both runtime capability and development experience.

Ah, yes, plugins are great. Pytest is written to make plugins easy.

LISAv2 global state/variable brings runtime troubles.

I don't agree. Firstly, we have different understanding on this goal. IMO, lisa v2 has a lot of built-in parameters, which pass through from command line and changed on somewhere by work arounds. There always variables need to be passed in, and may be changed by different purpose. This goal is not to elimate parameters, it's to make things sense, and easy to understand. If see your above troubles of global variables, all of them are in lisa test code. I know it's not good practice, and thank you to bring them out. Below is points how parameters handles better in lisa v3.

Minimize built-in parameters. If you see argparser.py, lisa v3 implements very limited built-in parameters. Comparing it with how it's implemented in lisav2, you will see what's significant improved.

Test variables are defined by users, not lisa. For example, vm_size must be a variable by chaos monkey testing as a variable, but xdp needs a fixed vm_size. It depends on user's test purpose to decide using variable or not.

Limit parameter changes on specified locations. For example, if vm_size is set by a variable, it can be changed by preparing phase of azure platform. No other place can change it.

Logging variable changes clearly. Use vm_size as an example again.

After runbook loaded with all variables, it dumps in log. So that user can check if there is any misconfiguration in runbook.

When preparing the environment, it may change vm_size there. The prepared vm_size is dumped here to understand any changes on vm_size.

When vm_size passes into arm_template, it's dumped again. The vm_size won't be changed any more after it's passed into arm template. It's a final gatekeeper to help identifying any unexpected changes.

I'm sorry, I'm struggling to follow this. Why are there so many places to replace the test variables? I think user's should have exactly one place to set their variables, and that should be as close to the test itself (so in the test's metadata, however that's implemented).

I'm considering going back to the drawing board, because I know that we need to not repeat the mistakes of LISAv2, and this must be maintainable. I have a tentative plan drafted to make LISAv3 a plugin for pytest, but am still working on it.

I really like what pytest can do, please go ahead to check if it can be a plug-in of pytest. At the same time, can we have pytest replace current unittest.TestCase? If so, test case developers can use new experience directly. We don't have many test cases today.

Update:
Here's a working demo using Pytest instead. (Note that my machine has SSH config setup to connect to a node via the name "centos", which won't work for you out of the box.)

Thanks! I think the Pytest based demo implements the majority of what we've implemented here, with 2.3% of the code.

Source lines of code for lisa/ subfolder on main branch:

SLOC    Directory       SLOC-by-Language (Sorted)
3182    top_dir         python=3182
1935    tests           python=1935
1123    sut_orchestrator python=1123
795     util            python=795
251     tools           python=251
113     parameter_parser python=113
35      features        python=35
25      notifiers       python=25

Totals grouped by language (dominant language first):
python:        7459 (100.00%)

7459 (total) - 1935 (tests) = 5,524

Source lines of code for pytest/ subfolder on andschwa/pytest branch:

SLOC    Directory       SLOC-by-Language (Sorted)
81      top_dir         python=81
46      testsuites      python=46

Totals grouped by language (dominant language first):
python:         127 (100.00%)

127 (total including example tests)

127/5,524 = 2.3%

squirrelsc · 2020-10-12T07:36:36Z

-class HelloWorld(TestSuite):
-    @TestCaseMetadata(
+class HelloWorld(LisaTestCase):
+    @LisaTestMetadata(


The Test is confusing. Test means much more than test case, it's hard to understand Test is subitem of Test case.

squirrelsc · 2020-10-12T07:38:03Z

-    "TestSuiteMetadata",
-    "TestCaseMetadata",
-    "TestSuite",
+    "LisaTestCase",


I need some guidances what kind of things worth to have a shortcut in __init__.

squirrelsc · 2020-10-12T07:47:57Z

    try:
-        exit_code = main()
+        # TODO: Turn off debugging when we ship this.
+        exit_code = asyncio.run(main(), debug=True)


Can it be loaded from debug flag or something else? It's not a reliable way. We may need this after shipped.

squirrelsc · 2020-10-12T07:56:44Z

    load_from_env(variables)
-    if hasattr(args, "variables"):
-        load_from_pairs(args.variables, variables)
+    load_from_pairs(user_variables, variables)


I hit errors if no variable defined. how it's fixed?

squirrelsc · 2020-10-12T07:57:22Z

+)
+from lisa.util.logger import get_logger
+
+log = get_logger("runner")


squirrelsc · 2020-10-12T09:33:37Z


-        if not isinstance(self._process, ExecutableResult):
+        if self._result is None:
+            # if not isinstance(self._process, ExecutableResult):


remove this line?

squirrelsc · 2020-10-12T09:34:00Z

-            self._process = result
-        else:
-            result = self._process
+            # TODO: The spur library is not very good and leaves open


squirrelsc · 2020-10-12T09:42:53Z


    def is_running(self) -> bool:
-        if self._running:
+        if self._running and self._process:


How about set _running to False also, when _process is cleaned up?

squirrelsc · 2020-10-12T09:59:31Z

-    total_elapsed: float = 0
-
-
-class Action(metaclass=ABCMeta):


I will add artifact builder soon. It handles a lot of things before environment provisioning, and user may want to see progress and full picture in a diagram. That action can help tracking progress and provide such a picture. Anyway, I can add it later, when we implementing test case management portal.

squirrelsc · 2020-10-12T10:04:38Z

                use_new_environment=case_use_new_env,
            )
        ],
+        environment=generate_runbook(**kwargs),


Move it into the construction will hide default value of schema.Runbook. The environment is optional, so that default value should be covered also. The platform is mandatory, so it's in construction.

Thank you, Andy! I will do cherry pick on this PR, to remove user experience, and project management related changes.

squirrelsc · 2020-10-26T03:45:59Z

Cherry picked in #1069 . Thank you, Andy!

Run Python in developer mode

305f4e5

The `-X dev` option reveals useful information such as leaked system resources.

lpereira reviewed Oct 5, 2020

View reviewed changes

Comment thread CONTRIBUTING.md

lpereira reviewed Oct 5, 2020

View reviewed changes

Comment thread lisa/tests/test_testsuite.py

andyleejordan force-pushed the andschwa/asyncio branch from 61ac746 to ab37ab5 Compare October 6, 2020 03:46

andyleejordan added 16 commits October 6, 2020 01:57

Add test to make all

79c875b

Forgot this.

Add info on Async IO to contributing guidelines

1e4c141

Clean up parse_args

111d17d

Make LisaRunner not subclass Action

3fd10c8

It was not being used for anything useful.

Rename LisaRunner to Runner since we only have one

af9addf

Also rename the files

4fa6d99

Rename start to run for Runner

ac46da0

Since there is no longer a corresponding `stop` (and when there was a `stop` it didn’t actually do anything).

Delete fluffy _create_test_results

bfaecf9

Since it’s really just a list comprehension. Also add a bunch of notes identifying things to investigate further.

Simplify generate_test_runbook

170cf1c

Since it now always expects an `env_runbook` we can just construct it on-the-fly.

Clean up tests

e446320

Temporarily rename test_runner back to test_lisarunner

b333442

Because an implicit alphabetical dependency in the test module names breaks the tests.

juhlee-microsoft added the 🆕 LISAv3 Incubation work for the next version of LISA label Oct 6, 2020

andyleejordan force-pushed the andschwa/asyncio branch 2 times, most recently from eb05bdf to b333442 Compare October 6, 2020 21:03

andyleejordan added 3 commits October 6, 2020 18:29

Fix some types in process and shell

1d78412

There’s more work to do because this was using duck typing. It’s a little better now, but it’s just a massive wrapper for a library that’s just another wrapper for Python’s actual APIs (which are just wrappers for the system’s APIs).

Manually close the fds which spur leaks

7cbe0e6

Rename TestSuite to LisaTestCase to avoid confusion

806f662

Since we’re reusing the unittest module’s `TestCase` class, we should not introduce confusing new nomenclature like `TestSuite`, since a “suite” is different from a “case” in usage.

andyleejordan force-pushed the andschwa/asyncio branch from 6f4196a to 806f662 Compare October 7, 2020 21:40

andyleejordan added 3 commits October 7, 2020 15:37

Rename TestCaseMetadata to LisaTestMetadata for consistency

c5bac19

Rename TestSuiteMetadata to LisaTestCaseMetadata for consistency

86d84e2

Rename _all_suites and _all_cases for consistency

5a914a0

Also rename (and delete where appropriate) the respective functions (and start cleaning them up, though they need more work).

andyleejordan added 4 commits October 7, 2020 16:38

Rename self.suite to self.case for consistency

b220dca

And start updating other functions.

Don’t subclass ABCMeta when unneeded

e4ee0b3

`LisaTestCase` is _not_ an abstract base class, it’s just a base class (with a lot of implementation) and defines no abstract methods. So we remove this.

Rename TestCaseRuntimeData to LisaTestRuntimeData

1967e7d

Again for consistency.

Simplify LisaTestCaseMetadata and LisaTestRuntimeData

cdb99b2

By using @DataClass. Also, this actually a case for multiple inheritance instead of abuse of `__getattr__`.

andyleejordan requested a review from squirrelsc October 9, 2020 19:15

andyleejordan marked this pull request as ready for review October 9, 2020 19:15

squirrelsc reviewed Oct 12, 2020

View reviewed changes

squirrelsc previously approved these changes Oct 20, 2020

View reviewed changes

squirrelsc self-requested a review October 23, 2020 13:55

squirrelsc mentioned this pull request Oct 26, 2020

Fixing anti-patterns (cherry picks from PR 1039) #1069

Merged

squirrelsc closed this Oct 26, 2020

LiliDeng deleted the andschwa/asyncio branch November 6, 2025 06:03



		@TestSuiteMetadata(
		@LisaTestCaseMetadata(

Conversation

andyleejordan commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squirrelsc commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyleejordan commented Oct 12, 2020

Uh oh!

andyleejordan commented Oct 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squirrelsc commented Oct 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andyleejordan commented Oct 2, 2020 •

edited

Loading

squirrelsc commented Oct 12, 2020 •

edited

Loading