Test pyrdp-mitm.py and pyrdp-player.py in the CI

[Res260]

Pyrdp-player.py is tested in headless mode using the replay from the same session as the current integration test.

I'm unsure if its a good idea to add --test to the program, but I don't see any other alternative. I'd like to hear ideas if you have any.

Note: when #191 is merged, this might need to be updated to take into account the logging configuration file

[alxbl]

I don't think we should be adding test-only logic to the codebase just to accomodate integration tests. In #191 I'm refactoring most of the pyrdp-mitm script to be calls to various modules so maybe you could base this work on that instead? If we're not actually running a connection through the MITM, I have a hard time seeing the point of just running the script through its imports up to but excluding the actual heavy lifting logic of the MITM. The player is already exercising most of the parsers during replay, isn't it?

I think most of the MITM stuff that can't be covered through the player should be unit tested. Proper integration tests for the MITM are going to be a lot of work.

[obilodeau]

I disagree with the --test approach. I see that @alxbl just beat me to a very good comment so I'm going to leave it at that.

The player stuff is good though. If you want to move fast, remove the mitm bits and it is mergeable.

[Res260]

If we're not actually running a connection through the MITM, I have a hard time seeing the point of just running the script through its imports up to but excluding the actual heavy lifting logic of the MITM. The player is already exercising most of the parsers during replay, isn't it?

@alxbl I thought the same thing, but the bug I introduced yesterday where I locked pyopenSSL to the wrong version (removing an important feature) would not have been caught through the player testing. It can only be caught with testing of the initialization of pyrdp-mitm.py OR unit tests.

Since I don't have time/motivation to do actual unit tests, I thought the simplest way to catch this kind of mistakes was this switch. I agree that test-only logic on the program sucks, but the alternative (for now) is to remove the test altogether, which imo is worse than having the switch (which also clearly indicate its purpose).

I'll remove the test if you still feel the same way!

[alxbl]

but the alternative (for now) is to remove the test altogether, which imo is worse than having the switch (which also clearly indicate its purpose).

I disagree. The switch adds complexity for the sake of saving time and is a public facing API which adds noise to an already long list of switches that make the help message more verbose. The main problem here is not having unit tests for this kind of thing. I'd rather have an issue open and fix it once, properly, than add test-specific logic. I agree with integration tests, but they should be done properly (like the player part) and tests should not leak into the actual codebase.

[Res260]

Could an alternative be to have a function in pyrdp-mitm.py called initializeMitm() that does everything the test currently covers and do a separate test that simply calls this method in pyrdp-mitm.py?

[alxbl]

I would say to take a look at 3b78221 and see how pyrdp-mitm.py looks like now. See if you can fit your test without actually touching the mitm launcher, and if it looks too complex, just drop the MITM test for now. We're going to add a basic suite of unit tests soon.

[Res260]

I refactored using your changes, and now simply call configure(). As said in the file header doc, this can be improved, but I think for now it still bring value.

@alxbl Is it normal I get this message?

Ref: https://github.com/GoSecure/pyrdp/runs/537935232?check_suite_focus=true
It seems like the directory pyrdp_output is not created if it doesnt exist.

[alxbl]

It seems like the directory pyrdp_output is not created if it doesnt exist.

Yeah, the line https://github.com/GoSecure/pyrdp/blob/master/pyrdp/mitm/cli.py#L178 should come before configureLoggers(), because the latter will attempt to create the log directory, but pyrdp_output hasn't been created yet.

You can fix it in this PR if you want, otherwise I'll push directly to master.

[Res260]

I fixed it @alxbl !

[alxbl]

I'm happy with these changes. I'd like to see a proper unit test suite since the MITM test is not really an integration test IMO as that would imply spinning up the server, accepting a connection, exercising some of the features, etc.

In any case we can move the test to be part of an eventual unit testing suite so I think this can be merged now. I'll let @obilodeau have the last word.

[Res260]

@alxbl interesting, I consider this an integration test because 1) it tests more than one "thing" at the same time and 2) it has side effects but 3) does not test a complete use case for a user

I call what youre describing an end to end test :o

[obilodeau]

Since we zipped pcaps, we should zip the test_session as well. This should be done for checkout tree size and consistency.

Some parts of this patch now look like an IDE "Organize imports"... I would have frowned on it but since they introduce a cleanup in the mitm case and a fix in the player case it's ok. 👍

[Res260]

@obilodeau

Since we zipped pcaps, we should zip the test_session as well. This should be done for checkout tree size and consistency.

I'd like ideas as to how to do it. Since there is no mechanism for pyrdp-player to dezip a file, it needs to be done before. So I see two ways to do that:

1. A python script. This has the downside of not directly testing pyrdp-player.py, which was previously possible.
2. An additionnal step in the CI. It is trivial for ubuntu, but requires some powershell script for windows, meaning I would most likely either add a .ps script in the pyrdp project, or have a very uglily formatted github workflow file.

Thoughts?

Some parts of this patch now look like an IDE "Organize imports"... I would have frowned on it but since they introduce a cleanup in the mitm case and a fix in the player case it's ok. 👍

Yeah I keep organize imports on commit since there is a lot of unused imports when it's not used.

[Res260]

@alxbl if you have an opinion on my previous comment it would be appreciated :)

[alxbl]

@alxbl if you have an opinion on my previous comment it would be appreciated :)

I haven't looked into this in-depth, but most CI pipelines include tasks for extracting zip files, This shouldn't be something done with OS-specific commands. A quick look around the action marketplace has shown me this:

https://github.com/DuckSoft/extract-7z-action/

Also, there might even be a built-in action for extracting zip files...
Actions Ref: https://help.github.com/en/actions

Another thing that Github actions can do is cache test data, so that you won't have to always re-extract it.

[Res260]

I forgot about the marketplace... Thanks for that. I now extract the zip which contains both files as an additionnal CI step. I think it should be good to be merged now!

[obilodeau]

Please note that the bundled zip is a mistake. An oversight on my part. Every time we will add a file to that zip it will change the zip and create new objects in .git/. This will cause pulls to the tree to consume more bandwidth and will make the tree bigger. We should zip large files that benefit from compression individually so existing tests' git object don't change if we add new compressed test cases.

Since I'm not sure how often we are going to add files in there I left things as is. This merge was already delayed enough. I'm going to add a note that next time we add a file test that would benefit from being compressed, we split things apart.