Fix - Broken tests #3013

jbaptperez · 2024-07-06T17:06:26Z

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 · 2024-07-06T18:06:30Z

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 · 2024-07-09T21:06:50Z

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 · 2024-07-22T08:50:39Z

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 · 2024-08-10T14:15:23Z

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

elegantmoose · 2024-08-16T20:09:09Z

@jbaptperez back on this, sorry for delay

uruwhy · 2024-09-24T16:29:29Z

app/api/rest_api.py

@@ -43,7 +43,7 @@ async def enable(self):
        self.app_svc.application.router.add_route('*', '/api/rest', self.rest_core)
        self.app_svc.application.router.add_route('GET', '/api/{index}', self.rest_core_info)
        self.app_svc.application.router.add_route('GET', '/file/download_exfil', self.download_exfil_file)
-        self.app_svc.application.router.add_route('GET', '/{tail:(?!plugin/).*}', self.handle_catch)
+        self.app_svc.application.router.add_route('GET', '/{tail:(?!plugin|api/v2/).*}', self.handle_catch)


Suggested change

self.app_svc.application.router.add_route('GET', '/{tail:(?!plugin|api/v2/).*}', self.handle_catch)

self.app_svc.application.router.add_route('GET', '/{tail:(?!plugins/|api/v2/).*}', self.handle_catch)

To maintain the previous path condition (also, it should be plugins/ rather than the original plugin/ value since the plugin paths use plugins/

Thanks.
I fixed up the original commit with the plugin to plugin/ change.

However, I put the plugin/ to plugins/ into a dedicated commit.

I saw a lot of /plugin/... endpoints (global RegEx search: add_route.+plugin/).
Are you sure the change that adds an s is correct?

No, good catch. I was going off the URLs in my browser when clicking different plugins, but the ones in add_route don't have the s as you pointed out. We'll just do plugin/

OK. I removed the commit and rebased my branch on the new master.

app/api/v2/handlers/health_api.py

uruwhy · 2024-09-24T19:23:40Z

app/objects/c_operation.py

-        data_svc_handle = BaseService.get_service('data_svc')
        seeded_facts = []
        if self.source:
-            seeded_facts = await data_svc_handle.get_facts_from_source(self.source.id)
+            seeded_facts = await knowledge_svc_handle.get_facts(criteria=dict(source=self.source.id))


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?

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.

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

Updates the pytest command to include the asyncio mode (required).

Jinja templates where not initialized in the test server.

A catch-all route was set in RestApi class before adding API v2 routes.

Method TestHealthApi.test_get_health. The new "access" field was not taken into account.

Method TestHealthApi.test_unauthorized_get_health. Empty access table was not taken into account.

Method TestPayloadsApi.test_get_payloads. Adds missing payloads routes. Updates the test to retrieve payloads (real temporary files).

Function test_access_denied. Deleted because there is no GET "/enter" endpoint. A catch-all route redirects "/enter" to "/".

Function test_custom_rejecting_login_handler. Using an endpoint that requires an authentication to check the redirection to the login page ("/").

Function test_home (only in CI/CD). The "index.html" template was missing in "plugins/magma/dist". Adds Node.js 20 (active LTS) dependency to the CI/CD. Adds "npm install" and "npm run build" commands to the "Install dependencies" step.

This reverts commit 3ad8849. The reverted commit broke tests/objects/test_link.py tests.

Makes it require authenticated users. Simplifies back the management of the returned "access" field.

jbaptperez force-pushed the fix/tests branch from de12574 to 768bf7b Compare July 6, 2024 17:40

jbaptperez marked this pull request as ready for review July 6, 2024 23:24

jbaptperez changed the title ~~Fix broken tests~~ Fix - broken tests Jul 6, 2024

jbaptperez changed the title ~~Fix - broken tests~~ Fix - Broken tests Jul 7, 2024

jbaptperez force-pushed the fix/tests branch 2 times, most recently from c8d948b to 44203bd Compare July 15, 2024 20:18

L015H4CK mentioned this pull request Jul 22, 2024

Add API functionality to update planners #3020

Merged

8 tasks

jbaptperez force-pushed the fix/tests branch from 44203bd to 4cfe9e0 Compare August 10, 2024 13:46

jbaptperez mentioned this pull request Aug 10, 2024

Fix - Broken tests mitre/atomic#44

Open

8 tasks

jbaptperez force-pushed the fix/tests branch from 39b36ab to 02af67f Compare August 17, 2024 12:29

uruwhy reviewed Sep 24, 2024

View reviewed changes

app/api/v2/handlers/health_api.py Outdated Show resolved Hide resolved

jbaptperez force-pushed the fix/tests branch from 02af67f to 19f568d Compare September 24, 2024 19:16

uruwhy reviewed Sep 24, 2024

View reviewed changes

jean-baptiste-perez-bib added 11 commits September 25, 2024 06:49

Updates CONTRIBUTING.md

9709d37

Updates the pytest command to include the asyncio mode (required).

Fixes crash when running API v2 tests

8154859

Jinja templates where not initialized in the test server.

Fixes API v2 routes unreachable in tests

a385e22

A catch-all route was set in RestApi class before adding API v2 routes.

Fixes a broken test (test_health_api.py)

852ebde

Method TestHealthApi.test_get_health. The new "access" field was not taken into account.

Fixes a broken test (test_health_api.py)

8c4a546

Method TestHealthApi.test_unauthorized_get_health. Empty access table was not taken into account.

Fixes a broken test (test_payloads_api.py)

bb70660

Method TestPayloadsApi.test_get_payloads. Adds missing payloads routes. Updates the test to retrieve payloads (real temporary files).

Fixes a broken test (test_core_endpoints.py)

77b4247

Function test_access_denied. Deleted because there is no GET "/enter" endpoint. A catch-all route redirects "/enter" to "/".

Fixes a broken test (test_core_endpoints.py)

c664db8

Function test_custom_rejecting_login_handler. Using an endpoint that requires an authentication to check the redirection to the login page ("/").

Fixes a broken test (test_core_endpoints.py)

54c931c

Function test_home (only in CI/CD). The "index.html" template was missing in "plugins/magma/dist". Adds Node.js 20 (active LTS) dependency to the CI/CD. Adds "npm install" and "npm run build" commands to the "Install dependencies" step.

Fixes a broken flake8 test (payload_api.py)

fb1180d

Fixes marshmallow warnings (payload_schemas.py)

4d2763a

jean-baptiste-perez-bib added 2 commits September 25, 2024 06:49

Reverts "retrieve seeded facts directly from source" (test_link.py)

2322579

This reverts commit 3ad8849. The reverted commit broke tests/objects/test_link.py tests.

Updates /api/v2/health endpoint

1d3ccc3

Makes it require authenticated users. Simplifies back the management of the returned "access" field.

jbaptperez force-pushed the fix/tests branch from 19f568d to 1d3ccc3 Compare September 25, 2024 04:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix - Broken tests #3013

Fix - Broken tests #3013

jbaptperez commented Jul 6, 2024 •

edited

Loading

jbaptperez commented Jul 6, 2024 •

edited

Loading

jbaptperez commented Jul 9, 2024

L015H4CK commented Jul 22, 2024

jbaptperez commented Aug 10, 2024

elegantmoose commented Aug 16, 2024

uruwhy Sep 24, 2024 •

edited

Loading

jbaptperez Sep 24, 2024

uruwhy Sep 24, 2024

jbaptperez Sep 25, 2024

uruwhy Sep 24, 2024

jbaptperez Sep 24, 2024

uruwhy Sep 24, 2024

	self.app_svc.application.router.add_route('GET', '/{tail:(?!plugin\|api/v2/).*}', self.handle_catch)
	self.app_svc.application.router.add_route('GET', '/{tail:(?!plugins/\|api/v2/).*}', self.handle_catch)

Fix - Broken tests #3013

Are you sure you want to change the base?

Fix - Broken tests #3013

Conversation

jbaptperez commented Jul 6, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

jbaptperez commented Jul 6, 2024 • edited Loading

jbaptperez commented Jul 9, 2024

L015H4CK commented Jul 22, 2024

jbaptperez commented Aug 10, 2024

elegantmoose commented Aug 16, 2024

uruwhy Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

jbaptperez Sep 24, 2024

Choose a reason for hiding this comment

uruwhy Sep 24, 2024

Choose a reason for hiding this comment

jbaptperez Sep 25, 2024

Choose a reason for hiding this comment

uruwhy Sep 24, 2024

Choose a reason for hiding this comment

jbaptperez Sep 24, 2024

Choose a reason for hiding this comment

uruwhy Sep 24, 2024

Choose a reason for hiding this comment

jbaptperez commented Jul 6, 2024 •

edited

Loading

jbaptperez commented Jul 6, 2024 •

edited

Loading

uruwhy Sep 24, 2024 •

edited

Loading