Skip to content

Conversation

@asmodehn
Copy link

@asmodehn asmodehn commented Apr 16, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #298

Description of Changes

Added sample_config and sample_connection in tests.
Adjust the tests to pass (OK on my machine, still WIP for CI...)

@asmodehn asmodehn marked this pull request as draft April 16, 2025 12:49
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@asmodehn there's a conflict with master.

@asmodehn
Copy link
Author

@nemesifier From what I can tell, these test failures come from sample_config and sample_connection tests that I have added to demonstrate another problem...

However these were not tested before... and the issue seems somewhat similar to openwisp/openwisp-controller#442, by which I mean "controller tests fail when accompanied by firmware_upgrader", albeit for other reasons...

So, I see few possible outcomes for this pull request:

  • keep fixing everything in this PR (it would likely take some time, and synchronization with controller changes...)
  • remove the tests from sample_config and sample_connection (but I feel we should keep them and actually get them to work eventually...)
  • only test openwisp2.sample_firmware_upgrader with SAMPLE_APP=1 (as it was before actually) by specifying it in ./runtests.py, and create an issue in here/in controller to get controller tests to work with firmware_upgrader, and deal with that as a different issue.

I'd favor the latter, but let me know which way you want to go, or if you see another possible option ?

Sadly, I don't think I will be very resourceful to fix these controller failing tests. The errors are regarding the database usage of the controller, and I don't know enough about this topic...

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@asmodehn can you update your openwisp-utils to the current latest master and run openwisp-qa-format on your branch and update this PR please?

This is due to:

openwisp/openwisp-utils#456
openwisp/openwisp-utils@c0bde3c

@asmodehn
Copy link
Author

@nemesifier Somehow openwisp-qa-format didn't do the job, I had to run black manually to replace quotes... I m not sure why that was the case...

@asmodehn asmodehn marked this pull request as ready for review May 27, 2025 09:30
@asmodehn
Copy link
Author

./run-qa-check and ./runtest.py are now passing for me (I didn't follow if something happened on the controller master recently to make it work ?).

Anyway, I m putting this "ready for review" so we verify the tests pass in CI, then I can do another pass to minimize the changes with master...

@nemesifier
Copy link
Member

nemesifier commented May 27, 2025

@nemesifier Somehow openwisp-qa-format didn't do the job, I had to run black manually to replace quotes... I m not sure why that was the case...

@asmodehn Make sure your openwisp-utils is up to date with the latest master, you may need to uninstall it and reinstall it.

@asmodehn asmodehn force-pushed the extended_controller_apps_tests branch 2 times, most recently from 2443847 to a3b2f02 Compare May 27, 2025 14:52
@asmodehn
Copy link
Author

SAMPLE_APP=1 coverage run ./runtests.py --parallel --exclude-tag=selenium_tests still failing:

Ran 720 tests in 72.204s

FAILED (failures=47, errors=1)

I ll have a look at these soon...

@asmodehn asmodehn closed this May 27, 2025
@asmodehn
Copy link
Author

I don't remember closing this one... sorry for the noise...

@asmodehn asmodehn force-pushed the extended_controller_apps_tests branch from 8cf4f08 to 7835686 Compare July 30, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To do (general)

Development

Successfully merging this pull request may close these issues.

[feature] Have tests (and more?) support an extended controller by parameterizing config viewnames

2 participants