Skip to content

Conversation

@pazzarpj
Copy link
Collaborator

Features:

  • Readable, less mental overhead to workout what is going on
  • Explicit TestListType declaration. You know exactly which test list class you are working with
  • Full auto_complete, type hinting allows for autocomplete on the object
  • No metaclass magic​. The implementation is quite simple (unlike my other metaclass magic implementations)
  • No Decorators
  • Backward compatible, can easily add back to existing tests to replace the context_data on a staged approach
  • Forced test scoping, test data cannot be shared across test lists, only those up the stack
  • If there are duplicate class matches, it will use the test list closest to it in the stack.

Implementation Details:

  • I have hooked into the inspect module and type annotations.
  • In the sequencer
  • At the beginning of the test (before all the setup methods are called) I create a fresh dictionary.
  • I populate the dictionary with the test lists it encounters along the way.
  • Instead of just calling active_test.test(), I first inspect active_test.test's type annotations.
  • I then key, value match the type annotated parameters with values stored in the dictionary
  • If any values are missing it will throw a test error
  • If there are duplicate matches of a class (eg using The same list type at two different levels), it will match to the test list closest to it in the stack.

@pazzarpj
Copy link
Collaborator Author

Black formatting errors raised in psf/black#1708

@clint-lawrence
Copy link
Collaborator

Hello Stranger!
Thanks for having a crack. I really should have created an issue here when I started throwing these ideas around a week or so ago. Over the weekend I'll try and pull together my other concepts to compare. In general this looks promising. If we were to go ahead with this approach, I would change the passed in attributes so that they are not TestList instances, but the specific data being shared. However, might have implications for the lookup process.

I've gone back and forth with 3 general concepts:

  • The sequencer needs more logic to look inside test list/class (what you've done)
  • The TestList/TestClass to all the work, but rely on being able to query the sequencer context stack. (see example below)
  • The TestList/TestClass just keep references to each other. This is possible, but makes it much hard to ensure the scoping is maintained. With knowing the context stack, you could pass data from a test list to a test that runs after that test list has completed. Or have a test that tries to access data from a test list that hasn't been entered yet. I've ruled this out.

Below is part of an idea I was kicking around. The idea is to inject a reference to the sequencer to TestList and TestClass. Instead of using a direct reference like I have there, I would actually create some kind of proxy that only exposes the api which the sequencer want to provide. My gut feel is that this is a good general approach that keep the coupling nicely defined, but with potential to expand when needed for other purposes. (e.g. adding a get_script_params interface and getting rid of context_data entirely). However this doesn't allow for the pytest magic style function params. Any thoughts?

For this shared data idea, the test script wouldn't directly access the _seq attribute, but there would be methods or properties on the test class (provided by the base class) which would use _seq (via a get_stack method?) to look for the shared data.

diff --git a/src/fixate/sequencer.py b/src/fixate/sequencer.py
index 6337246..f0aa215 100644
--- a/src/fixate/sequencer.py
+++ b/src/fixate/sequencer.py
@@ -260,7 +260,10 @@ class Sequencer:
                             data=top.current(),
                             test_index=self.levels(),
                         )
-                        top.current().enter()
+                        # inject sequencer reference
+                        test_list = top.current()
+                        setattr(test_list, "_seq", self)
+                        test_list.enter()
                         self.context.push(top.current())
                     else:
                         raise SequenceAbort("Unknown Test Item Type")
@@ -287,6 +290,7 @@ class Sequencer:
         """
 
         active_test = self.context.top().current()
+        setattr(active_test, "_seq", self)
         active_test_status = "PENDING"
         pub.sendMessage("Test_Start", data=active_test, test_index=self.levels())
         if active_test.skip:

@pazzarpj
Copy link
Collaborator Author

pazzarpj commented Sep 18, 2020

Perhaps we should move this discussion (or port the discussions to date) to an issue.

For your final point, The sequencer is already globally accessible through the RESOURCES["SEQUENCER"].
I can see removing global access to in preference to injecting it into the test class though, just that in it's current form it doesn't add anything to inject it.

get_script_params method could be implemented without touching the sequencer logic, unless we remove the global scoping of the sequencer as mentioned above.
I can't see how you would go about getting your type hinted/ autocomplete data through a generic function call. Too much python magic and the IDE just ignores it.

I agree that context_data needs to go, it has too many disadvantages.

As for limiting the exact parameter you are looking for in this PRs implementation. There are a few ways to go about this
1

def test(self, outer: TestListOuter.outer, outer_other_data: TestListOuter.other_data):

This doesn't work because the whole point is to share data between tests and this would only work if the value is already set and is mutable like a dict.

2

def test(self, outer: "TestListOuter.outer", outer_other_data: "TestListOuter.other_data"):

This can share data but the type hinting is stuffed

3

def test(self, outer: TestListOuter, other_data: TestListOuter):

Could use the name of the variable to indicate which parameter in the test list instance to parse.
This would break auto completion because outer would be an int but the type hinting would be a TestList

4

@test_data(outer="TestListOuter.outer", other_data="TestListOuter.other_data")
class TestBoth(TestClass):
    def test(self):
        print(self.test_data.outer)
        print(self.test_data.other_data)

Explicitly defines the data parsed but you lose type hinting and auto complete. The problem is that you have to reference by string the TestLists in the @test_data because the data might be immutable or not exist yet. Eg if TestListOuter.outer is an int, then by the time it runs it could be set to another number

5

@test_data(outer="TestListOuter.outer", other_data="TestListOuter.other_data")
class TestBoth(TestClass):
    def test(self, outer: int, other_data: dict):
        pass

This is probably the best alternative
This gives you explicit parameter declaration and the option to type hint the injected parameters. The problem is that you have to reference by string the TestLists in the @test_data because the data might be immutable or not exist yet. Eg if TestListOuter.outer is an int, then by the time it runs it could be set to another number

@clint-lawrence
Copy link
Collaborator

For your final point, The sequencer is already globally accessible through the RESOURCES["SEQUENCER"].
I can see removing global access to in preference to injecting it into the test class though, just that in it's current form it doesn't add anything to inject it.

I can't reply to everything right now, but yes, you hit on my motivation to suggest doing it that way. The RESOURCES dict needs to go eventually and this would be a baby-step in that direction.

@clint-lawrence
Copy link
Collaborator

@pazzarpj would be great to get your thoughts on issue #127. I think with your 5th proposal above might be a good overall approach.

@clint-lawrence
Copy link
Collaborator

Black formatting errors raised in psf/black#1708

@pazzarpj I just pinned the version of black on CI to work around this. If you continue on this PR you can grab that from master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants