Fix - Broken tests

[jbaptperez]

Description

Fixes:

• test documentation (updates CONTRIBUTONG.md according to new aiohttp requirements in the CLI)
• crash when starting API v2 tests (missing Jinja setup)
• unreachable API v2 GET routes (as POST, PUT and PATCH routes are) because of a catch-all route set in RestApi (v1) class before API v2 routes have been added
• failed tests
• linter issues
• CI/CD failures (node dependency was missing).

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

All tests:

# Creating and activating a virtualenv in a .venv directory.
python -m venv .venv
. .venv/bin/activate

# Removing conflicting dnspython in requirements-dev.txt...
sed -i '/dnspython==2.1.0/d' requirements-dev.txt # Linux
sed -i '' '/dnspython==2.1.0/d' requirements-dev.txt # macOS
pip install -r requirements.txt -r requirements-dev.txt

# Installing node files (the required `plugins/magma/dist/index.html` file is to be generated)...
npm --prefix plugins/magma install
npm --prefix plugins/magma run build

# Executing all tests...
pytest --asyncio-mode=auto

# Applying linters...
pre-commit run --all-files --show-diff-on-failure

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

[jbaptperez]

I could fix all test failures locally (macOS), but it still remains tons of warnings (see CI/CD).
I also fixed 2 CI/CD failures:

• Missing magma "index.html" in plugins/magma/dist and added the missing node part,
• A trivial flake8 bad style.

Now, the CI/CD still does not succeed: It looks like the SONAR_TOKEN environment variable is not set in GitHub.
I think we are really close to completely restore testing capabilities.

What do you think @elegantmoose and @clenk?
Any help?

[jbaptperez]

I tried to understand the remaining event loop warnings that appear when running tests, the ones that produce Task was destroyed but it is pending! or RuntimeError: Event loop is closed.

I focused on a single test that cause such warnings.
Below is the command to produce the issue:

pytest --asyncio-mode=auto --tb=short tests/services/test_app_svc.py::TestAppService::test_mark_agent_as_untrusted_running_operation

After investigating, I figured out that the "destroyed" asyncio tasks come from the application itself.
The application starts when the test method retrieves its fixtures.

That means when the test method start, It is already "too late" as the event loop has already a set of pending tasks.
It also means that even with a completely empty body, the test method would produce the same warnings.

@bleepbop and @christophert, you both have worked on the tests.
Do you think there is a proper way to fix that?
I mean, something like starting a real "test-only light" application that does not start like in a normal one (without pending tasks).

I could reduce (but not completely suppress) the amount of warning by just applying this function at the beginning or the end of the test (whatever):

async def cancel_pending_tasks():
    loop = asyncio.get_running_loop()
    current_task = asyncio.current_task(loop)
    pending_tasks = [task for task in asyncio.all_tasks(loop) if task is not current_task and not task.done()]
    for task in pending_tasks:
        task.cancel()

However, this is not a clean way to fix that specific issue.
Therefore, if you both don't have a solution to propose for now, I prefer stopping my investigations and let this pull request be merged (when the Sonar environment variable issue is solved, see above).

[L015H4CK]

Thanks for the great work!

I was working on some new features myself and ran into a lot of failing tests - wondering where they came from. Then I noticed they fail on the main branch as well. I just pulled your branch and the tests are fixed.

In my opinion, adding --asyncio-mode=auto to the pytest call is also important.

[jbaptperez]

While rebasing the work on the new master, I faced a regression introduced in commit 3ad8849 that broke some tests.

[elegantmoose]

@jbaptperez back on this, sorry for delay

[uruwhy]

It sounds like these changes from #2978 were causing issues in your tests - I'd like to avoid undoing changes from previous PRs and fix the corresponding tests if possible.

@elegantmoose @clenk thoughts?

[jbaptperez]

Correct, the changes broke the tests.

However, after passing a couple of hours trying to understand what was going on (using a debugger, etc.), I figured out that related data structure were not so similar and maybe the shortcut of #2978 cannot really be applied this way.

I don't master the code so here, I clearly need help to have a proper fix.

[uruwhy]

I see. Let me talk with some folks to see how we want to approach this. Will get back to you shortly

[uruwhy]

Alright, I figured out how to fix the 3 broken unit tests if we bring this change back:

--- a/tests/objects/test_link.py
+++ b/tests/objects/test_link.py
@@ -138,7 +138,7 @@ class TestLink:
         knowledge_base_r = event_loop.run_until_complete(knowledge_svc.get_relationships(dict(edge='has_admin')))
         assert len(knowledge_base_r) == 1
 
-    def test_create_relationship_source_fact(self, event_loop, ability, executor, operation, knowledge_svc, fire_event_mock):
+    def test_create_relationship_source_fact(self, event_loop, ability, executor, operation, data_svc, knowledge_svc, fire_event_mock):
         test_executor = executor(name='psh', platform='windows')
         test_ability = ability(ability_id='123', executors=[test_executor])
         fact1 = Fact(trait='remote.host.fqdn', value='dc')
@@ -149,6 +149,7 @@ class TestLink:
                               adversary=Adversary(name='sample', adversary_id='XYZ', atomic_ordering=[],
                                                   description='test'),
                               source=Source(id='test-source', facts=[fact1]))
+        event_loop.run_until_complete(data_svc.store(operation.source))
         event_loop.run_until_complete(operation._init_source())
         event_loop.run_until_complete(link1.create_relationships([relationship], operation))
 
@@ -161,7 +162,7 @@ class TestLink:
         assert len(fact_store_operation) == 1
         assert len(fact_store_operation_source[0].collected_by) == 2
 
-    def test_save_discover_seeded_fact_not_in_command(self, event_loop, ability, executor, operation, knowledge_svc, fire_event_mock):
+    def test_save_discover_seeded_fact_not_in_command(self, event_loop, ability, executor, operation, knowledge_svc, data_svc, fire_event_mock):
         test_executor = executor(name='psh', platform='windows')
         test_ability = ability(ability_id='123', executors=[test_executor])
         fact1 = Fact(trait='remote.host.fqdn', value='dc')
@@ -172,6 +173,7 @@ class TestLink:
                               adversary=Adversary(name='sample', adversary_id='XYZ', atomic_ordering=[],
                                                   description='test'),
                               source=Source(id='test-source', facts=[fact1, fact2]))
+        event_loop.run_until_complete(data_svc.store(operation.source))
         event_loop.run_until_complete(operation._init_source())
         event_loop.run_until_complete(link.save_fact(operation, fact2, 1, relationship))
 
diff --git a/tests/objects/test_operation.py b/tests/objects/test_operation.py
index d3d4ecc..be95d9d 100644
--- a/tests/objects/test_operation.py
+++ b/tests/objects/test_operation.py
@@ -427,6 +427,7 @@ class TestOperation:
 
     def test_facts(self, event_loop, app_svc, contact_svc, file_svc, data_svc, learning_svc, fire_event_mock,
                    op_with_learning_and_seeded, make_test_link, make_test_result, knowledge_svc):
+        event_loop.run_until_complete(data_svc.store(op_with_learning_and_seeded.source))
         test_link = make_test_link(9876)
         op_with_learning_and_seeded.add_link(test_link)

[uruwhy]

Those tests in particular need to manually store the operation fact sources in the data service - the data service loads fact sources on server startup, so I essentially had to replicate that behavior with the dummy fact sources for the unit test

[jbaptperez]

@uruwhy, thank you 👍
I applied your changes and all tests are fixed now.

For the CI/CD to work properly again, only one thing is missing: The missing value of the SONAR_TOKEN environment variable (see my previous comment).

[uruwhy]

I'm not sure i'm seeing the same sonar issue that you are, but I did also see some flake8 issues from the latest runs:

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

app/api/v2/managers/fact_source_manager.py:3:1: E302 expected 2 blank lines, found 1
app/service/data_svc.py:484:1: W293 blank line contains whitespace

[jbaptperez]

@uruwhy, thanks, I missed them. Fixed.

Now the SONAR_TOKEN missing value appears in the SonarCloud Scan task:

Set the SONAR_TOKEN env variable.

.github/workflows/quality.yml:64:

      - name: SonarCloud Scan
        uses: SonarSource/sonarcloud-github-action@49e6cd3b187936a73b8280d59ffd9da69df63ec9
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}  # Needed to get PR information, if any
          SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}

[uruwhy]

So PRs from forks don't have access to our github secrets, and we'd have to make some major workflow adjustments to accommodate this sort of thing. However, I looked through the coverage.xml manually and verified that all your new lines of code are covered

[elegantmoose]

Awesome work @jbaptperez . TY so much. 🥇

[elegantmoose]

Tested.

[jbaptperez]

@elegantmoose, note that this other PR completes test fixes with in the atomic plugin.

Please have a look to it.