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.

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

Add testing framework #3

Merged
merged 11 commits into from
Jun 6, 2020
Merged

Add testing framework #3

merged 11 commits into from
Jun 6, 2020

Conversation

jasonrute
Copy link
Collaborator

I've added a testing framework for the trio server. Basically I've created a mock lean process in Trio which runs according to a (linear) script. I've included a test for the issue we already fixed where Lean stops waiting after we return a current_tasks response too early.

Also, to facilitate easier debugging, I've added another level of debugging called debug_bytes. This prints the lean messages as they are sent over the channel. Since parse_response in the trio server often crashes, this is a way to get at the message it is crashing on. Also, it is useful for creating the mock lean scripts in the tests.

Curious to get your thoughts Patrick.

@jasonrute
Copy link
Collaborator Author

Ok, prematurely PRed this, but it is all fixed now.

@PatrickMassot
Copy link
Member

Nice! I don't have time to read it carefully tonight (it's 11pm here), but the description of the mock Lean process sounds like what I intended to do at some point. The style of the test script reminds me of Behavior Driven Development. You could have a look at pytest-bdd if you fancy that style.

@jasonrute jasonrute mentioned this pull request Apr 25, 2020
Copy link
Member

@PatrickMassot PatrickMassot left a comment

Choose a reason for hiding this comment

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

Thanks Jason, this is indeed essential and was on my TODO list as well. I left comments aiming to simplify things a bit.

test/test_trio_server/mock_lean.py Show resolved Hide resolved
test/test_trio_server/test_full_sync.py Outdated Show resolved Hide resolved
test/test_trio_server/mock_lean.py Show resolved Hide resolved
test/test_trio_server/mock_lean.py Show resolved Hide resolved
test/test_trio_server/test_full_sync.py Outdated Show resolved Hide resolved
@jasonrute
Copy link
Collaborator Author

I switched to pytest. I'll wait for your thoughts on my thoughts before making other changes here.

@jasonrute
Copy link
Collaborator Author

Just so you know, I haven't forgot about this. I got caught up in some other projects and personal stuff and I have to go to work today, but I'll get back to this soon. Thanks for the changes!

@PatrickMassot
Copy link
Member

There is no hurry at all, I have plenty to do while waiting.

@jasonrute
Copy link
Collaborator Author

Ok, I'm back at this. (It only took me a month.) I'll read through the code again and try to finish these PRs.

@jasonrute
Copy link
Collaborator Author

jasonrute commented Jun 3, 2020

Ok, added some more fixes, but I need to still merge (or rebase) master. I tried it earlier and ran into some errors in the tests that need to be fixed.

@jasonrute
Copy link
Collaborator Author

Ok, I think we have addressed all the concerns. @PatrickMassot can I get an approval on this? (Also, note that I did a rebase, so if you are going to do anything with this branch, be aware. I don't think you had any outstanding edits to this branch, so I thought it was safe.)

self.messages.extend(self.parse_message(m) for m in raw_messages)

async def assert_message_is_received(self, message_expected):
await trio.sleep(0.01)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this explicit sleep (and the one below)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a great question. I'm pretty sure it was necessary, but let me remember/investigate why and I'll add it as a comment to the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've addressed this in this commit (a585707). In particular,

  1. I moved the sleep command into the collect_stdin_messages to avoid duplicated code.
  2. I clearly explained what three goals the sleep command (in conjunction with get_data_nowait()) are intended to accomplish.
  3. I made the waiting time a parameter which could be changed. It might need to be extended if we are testing some complicated functionality which takes a while to compute.
  4. I left the default of 0.01 seconds. In my tests, 0.001 seconds also allowed the tests to pass but 0.0001 was too fast. I thought 0.01 seconds was short enough to not make the tests take too long, but also gave an order of magnitude buffer to account for slow machines.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand. The whole point of this async thing is to avoid guessing how long we should wait for answers from Lean. Why do we need any sleep? Is this only for tests, simulating some delay in answers, or is this going to be needed in actual uses of this lean-python client?

Copy link
Collaborator Author

@jasonrute jasonrute Jun 6, 2020

Choose a reason for hiding this comment

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

In short, it is only needed for tests because we don't want to wait forever for the Lean Python client to return information which it may never do.

Remember, this is "Mock Lean", so I'm simulating a fake Lean server process waiting for our Python client to send a request. A real Lean server would make a blocking call to read the data and wait as long as necessary. That could be accomplished via self.stdin.get_data() in collect_stdin_messages, but the problem is that I don't know if our client will send any requests and I could be left waiting forever. This is used to support LeanShouldGetRequest and LeanShouldNotGetRequest. Those test that either Lean receives a request (and it is a particular form) or that it doesn't receive a request, so I need the ability to stop waiting.

So what I'm doing here is waiting for our Python client for a little bit (enough time for our Python client to run and send a message if it is going to), and then checking if the message is sent in a non-blocking manner.

Another way I could possibly implement this is via a blocking call, self.stdin.get_data(), and then use a feature of Trio testing framework that has some way to stop execution of a blocked coroutine after a certain time. I think this might exist, however, I'd have to look into that more. It is not clear to me how to implement this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So like I said, an alternative way to do this would be with cancellation and timeouts. I would still need to look into how to do this more. Would you prefer that approach?

Copy link
Collaborator Author

@jasonrute jasonrute Jun 6, 2020

Choose a reason for hiding this comment

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

I could do something like this inside collect_stdin_messages:

with trio.move_on_after(wait_time_seconds):
    try:
        data = await self.stdin.get_data()
    except Cancelled:
        return None
   
self.partial_message = raw_messages.pop()
self.messages.extend(self.parse_message(m) for m in raw_messages)

Copy link
Collaborator Author

@jasonrute jasonrute Jun 6, 2020

Choose a reason for hiding this comment

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

Ok, I just pushed a commit using the above snippet. I think hope it will clear up a lot of the confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this expresses what's going on much more directly.

@PatrickMassot PatrickMassot merged commit b30c9a5 into master Jun 6, 2020
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.

2 participants