Speed up unit tests #1336

mssalvatore · 2021-07-19T19:07:38Z

What does this PR do?

Resolves #1125

This PR uses a number of tricks to speed up unit test execution:

Remove an unnecessary 1 second sleep from test_monkey.py.
Scope expensive fixtures at the session instead of test level.
Reduce calls to expensive functions
Avoid expensive imports where possible
Use pytest markers to filter out slow tests.

Results

Collection Time

Collection time is sped up by 34%

Total Run Time

Total run time for the entire suite is sped up by 30%

Option to skip slow tests

For some slow tests, it would cost more to speed them up than it would save. A nice workaround for this is to use pytest markers to mark them as slow. They can then be skipped by running pytest -m 'not slow'. This speeds up the total runtime by 60%, at the expense of skipping 14 tests.

PR Checklist

Have you added an explanation of what your changes do and why you'd like to include them?
Is the TravisCI build passing?
Was the CHANGELOG.md updated to reflect the changes?
~~Was the documentation framework updated to reflect the changes?~~

Testing Checklist

Added relevant unit tests?
Have you successfully tested your changes locally? Elaborate:

Tested by running unit tests
If applicable, add screenshots or log transcripts of the feature working

Calls to encrypt_string() result in calls to pyAesCrypt.encryptStream(). These calls are very slow (about .150ms). Modifying these tests to use static ciphertext instead of encrypting the file each time saves approximately 300ms when running the unit test suite.

This allows you to skip slow tests by running `pytest -m 'not slow'`.

WmiExploiter relies on impacket. Importing impacket is slow, which has a negative impact on the speed of pytest collection. SSHExploiter is much quicker to import.

The ZerologonExploiter relies on impacket. Importing impacket is slow (approximately .72s). By moving the import statement in zerologon tests and marking them as slow, the import (and tests) can now be skipped by running `pytest -m 'not slow'`.

codecov · 2021-07-19T19:08:10Z

Codecov Report

Merging #1336 (adb1006) into develop (81f7de7) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1336      +/-   ##
===========================================
- Coverage    40.31%   40.09%   -0.23%     
===========================================
  Files          475      474       -1     
  Lines        13874    13878       +4     
===========================================
- Hits          5593     5564      -29     
- Misses        8281     8314      +33

Impacted Files	Coverage Δ
monkey/monkey_island/cc/services/config.py	`53.33% <100.00%> (ø)`
monkey/tests/unit_tests/conftest.py	`100.00% <100.00%> (ø)`
.../unit_tests/monkey_island/cc/resources/conftest.py	`100.00% <100.00%> (ø)`
monkey/infection_monkey/exploit/wmiexec.py	`0.00% <0.00%> (-28.38%)`	⬇️
monkey/infection_monkey/exploit/tools/wmi_tools.py	`0.00% <0.00%> (-23.66%)`	⬇️
monkey/infection_monkey/exploit/tools/smb_tools.py	`0.00% <0.00%> (-15.11%)`	⬇️
monkey/infection_monkey/exploit/sshexec.py	`21.67% <0.00%> (+21.67%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81f7de7...adb1006. Read the comment docs.

VakarisZ · 2021-07-20T06:46:12Z

monkey/monkey_island/cc/services/config.py

@@ -360,7 +360,7 @@ def _encrypt_or_decrypt_config(config, is_decrypt=False):
                parent_config_arr = config_arr
                config_arr = config_arr[config_key_part]

-            if isinstance(config_arr, collections.Sequence) and not isinstance(config_arr, str):
+            if isinstance(config_arr, collections.abc.Sequence) and not isinstance(config_arr, str):


What's the rationale behind this?

VakarisZ · 2021-07-20T07:04:47Z

Did you do any more intricate profiling than just running the pytest? With --collect-only I get 326 tests collected in 2.72s. Without it I get 3 failed, 315 passed, 8 skipped, 2 warnings in 5.73s. Looks like about half of the time is spent collecting the tests 🤔I've ran the profiler and most time seems to be spent doing os.stat, not sure if and how we can optimize it.

shreyamalviya

Nice!

shreyamalviya · 2021-07-20T07:09:12Z

monkey/tests/unit_tests/monkey_island/cc/models/test_monkey.py

        # Emulate timeout - ttl is manually deleted here, since we're using mongomock and not a
        # real mongo instance.
-        sleep(1)


Why was this here in the first place?

¯\(ツ)/¯

ilija-lazoroski · 2021-07-20T07:29:03Z

monkey/tests/unit_tests/infection_monkey/telemetry/test_exploit_telem.py

@@ -2,7 +2,7 @@

 import pytest

-from infection_monkey.exploit.wmiexec import WmiExploiter
+from infection_monkey.exploit.sshexec import SSHExploiter


Why are we changing the exploiter ?

See commit messages: 2496ed0

mssalvatore · 2021-07-20T10:47:12Z

@VakarisZ

Did you do any more intricate profiling than just running the pytest?

I did.

With --collect-only I get 326 tests collected in 2.72s. Without it I get 3 failed, 315 passed, 8 skipped, 2 warnings in 5.73s.

Did this PR introduce failures for you?

Looks like about half of the time is spent collecting the tests. I've ran the profiler and most time seems to be spent doing os.stat, not sure if and how we can optimize it.

Most of the collection time is because of slow imports. The biggest one is, impacket. For the most part, we're not testing the code that uses impacket, but it still gets imported. I was able to avoid importing impacket if you run pytest -m 'not slow', but actually fixing the issue would involve a significant refactor of the ZeroLogon exploiter.

Some other imports are also noticeably slow, but due to some tight coupling in the system, it's not trivial to avoid those imports.

Below are some nicely formatted cProfile results from when pytest is run with --collect-only

mssalvatore added 12 commits July 19, 2021 07:46

Tests: Remove unnecessary sleep(1)

e12bc7a

Tests: Move contents of tests/conftest.py

2897755

Tests: Add monkeypatch_session fixture

5222230

Tests: Make flask_client a session-scoped fixture

162e375

Tests: Convert test_get_all_mitigations() from unittest to pytest

f0033d0

Tests: Mark slow tests with @pytest.mark.slow

ac52c30

This allows you to skip slow tests by running `pytest -m 'not slow'`.

Tests: Address mongomock deprication warnings

41cf0f0

Tests: Address deprecation warning in config.py

845c9d9

Tests: Use SSHExploiter instead of WmiExploiter in expliot telem tests

2496ed0

WmiExploiter relies on impacket. Importing impacket is slow, which has a negative impact on the speed of pytest collection. SSHExploiter is much quicker to import.

Tests: Mark ZeroLogon tests as slow

d9a1f22

The ZerologonExploiter relies on impacket. Importing impacket is slow (approximately .72s). By moving the import statement in zerologon tests and marking them as slow, the import (and tests) can now be skipped by running `pytest -m 'not slow'`.

Update the CHANGELOG

adb1006

mssalvatore requested review from ilija-lazoroski, VakarisZ and shreyamalviya July 19, 2021 19:07

VakarisZ reviewed Jul 20, 2021

View reviewed changes

VakarisZ approved these changes Jul 20, 2021

View reviewed changes

shreyamalviya approved these changes Jul 20, 2021

View reviewed changes

ilija-lazoroski reviewed Jul 20, 2021

View reviewed changes

ilija-lazoroski approved these changes Jul 20, 2021

View reviewed changes

mssalvatore merged commit 5f31822 into develop Jul 20, 2021

mssalvatore deleted the speed-up-unit-tests branch July 20, 2021 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up unit tests #1336

Speed up unit tests #1336

mssalvatore commented Jul 19, 2021 •

edited

Loading

codecov bot commented Jul 19, 2021 •

edited

Loading

VakarisZ Jul 20, 2021

mssalvatore Jul 20, 2021

VakarisZ commented Jul 20, 2021

shreyamalviya left a comment

shreyamalviya Jul 20, 2021

mssalvatore Jul 20, 2021 •

edited

Loading

ilija-lazoroski Jul 20, 2021

mssalvatore Jul 20, 2021

mssalvatore commented Jul 20, 2021

Speed up unit tests #1336

Speed up unit tests #1336

Conversation

mssalvatore commented Jul 19, 2021 • edited Loading

What does this PR do?

Results

Collection Time

Total Run Time

Option to skip slow tests

PR Checklist

Testing Checklist

codecov bot commented Jul 19, 2021 • edited Loading

Codecov Report

VakarisZ Jul 20, 2021

Choose a reason for hiding this comment

mssalvatore Jul 20, 2021

Choose a reason for hiding this comment

VakarisZ commented Jul 20, 2021

shreyamalviya left a comment

Choose a reason for hiding this comment

shreyamalviya Jul 20, 2021

Choose a reason for hiding this comment

mssalvatore Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

ilija-lazoroski Jul 20, 2021

Choose a reason for hiding this comment

mssalvatore Jul 20, 2021

Choose a reason for hiding this comment

mssalvatore commented Jul 20, 2021

mssalvatore commented Jul 19, 2021 •

edited

Loading

codecov bot commented Jul 19, 2021 •

edited

Loading

mssalvatore Jul 20, 2021 •

edited

Loading