Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix macOS runner and reenable it #2442

Closed
wants to merge 5 commits into from

Conversation

MattHag
Copy link
Collaborator

@MattHag MattHag commented Apr 20, 2024

Related #1244

@MattHag MattHag force-pushed the fix_macos_setup branch 2 times, most recently from f3f7404 to 13289d1 Compare April 20, 2024 20:41
@MattHag
Copy link
Collaborator Author

MattHag commented Apr 21, 2024

Failed tests seem to be due to too many imports caused by some "unit tests" which basically rely on every module.

@MattHag MattHag force-pushed the fix_macos_setup branch 4 times, most recently from 52a60c0 to 89b47c6 Compare April 21, 2024 13:02
MattHag added 3 commits April 21, 2024 15:03
hidapi is installed via brew and resides in /opt/homebrew, which
is not part of Python search path.
@pfps
Copy link
Collaborator

pfps commented Apr 21, 2024

So the MacOS test harness is failing? The solution is to not enable testing on MacOS or fix the harness, not to remove test that work on Linux.

@MattHag
Copy link
Collaborator Author

MattHag commented Apr 21, 2024

No they don't fail, they cause segfaults on import of a dependency. As these tests import everything, as the underlying code is still not ready for proper unit tests (a unit is a module and not 10 modules), it will always be a pain to update them, when the code is further simplified.

@pfps
Copy link
Collaborator

pfps commented Apr 21, 2024

Segfaulting is a failure of the harness.

@pfps
Copy link
Collaborator

pfps commented May 5, 2024

The second commit is fine. But the way forward is not to remove tests that work in Linux but to set things up in MacOS so that all tests work there.

@MattHag
Copy link
Collaborator Author

MattHag commented May 5, 2024

The problem with these tests is, that they load too many dependencies (still a problem of solaar, which I am steadily trying to improve), where at least one is not compatible with macOS. Instead of steadily adding tests and finding bugs with macOS, it is now necessary to fix all new dependencies loaded by the test. I don't know how many parts fail and need to be worked on in order to fix them.
Unfortunately, these tests are no unit tests which tackle a module, but load probably every module of Solaar. Thus, it is no longer possible to gradually add tests, which work for both and fix issues in small iterations, when adding a test as long as these tests depend on basically everything. That's exactly the reason, why I say, they have been added too early. Solaar needs to be testable on a module per module basis.

@pfps
Copy link
Collaborator

pfps commented May 5, 2024

Then the way forward is to fix things so that they work on MacOS but not just remove the tests.

@MattHag
Copy link
Collaborator Author

MattHag commented May 5, 2024

Yes, anyone can pick this up and fix it. Don't know if I am motivated enough in the near future to do a much harder work to get it up and running again. Might need to sit there and wait until solaar has fewer dependencies between its modules.

@MattHag
Copy link
Collaborator Author

MattHag commented May 16, 2024

Replaced by #2490

@MattHag MattHag closed this May 16, 2024
@MattHag MattHag deleted the fix_macos_setup branch May 26, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants