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

Add back in explicit windows and mac testing #953

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Jul 30, 2024

This is probably only a draft pull request, I was curious to see if it could help shed light on on #951. Adding windows os (and mac) back into CI OS testing matrix

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 3, 2025

Closing this one as seems like over-responding to one particular incident and greatly increasing our test count

@paulf81 paulf81 closed this Mar 3, 2025
@rafmudaf
Copy link
Collaborator

rafmudaf commented Mar 5, 2025

fwiw I think it's worth including Mac and Windows in the test matrix even if it's only for a subset. For example, you could run the example suite on all three systems but run the tests only on ubuntu. It would just be good to know that something is running on each OS.

@misi9170
Copy link
Collaborator

misi9170 commented Mar 5, 2025

fwiw I think it's worth including Mac and Windows in the test matrix even if it's only for a subset. For example, you could run the example suite on all three systems but run the tests only on ubuntu. It would just be good to know that something is running on each OS.

This feels like a reasonable compromise (examples only on ubuntu, since those take a while, and tests on all three OSs, since those are faster). I'll reopen the PR and make those updates.

@misi9170 misi9170 reopened this Mar 5, 2025
@misi9170
Copy link
Collaborator

misi9170 commented Mar 5, 2025

After readding macOS and windows tests, it appears that the only failing tests are on Windows using python 3.9; and moreover, they are all related to using parallel operations (most are explicit, but note that LayoutOptimizationRandomSearch also defaults to parallelized operation). Evidently, everything works fine on python 3.10 and higher on Windows.

So, there are a couple of options here:

  • Work on creating a fix so that the tests pass with python 3.9 and windows
  • Drop support for python 3.9 (which will be at end of life later this year)

@rafmudaf
Copy link
Collaborator

rafmudaf commented Mar 5, 2025

Just a heads up @misi9170 that a description of the issue is at #951 (comment). Based on their response, it looks like like @RHammond2 agrees with requiring Python 3.10+. I could go either way on that.

In addition to the suggestions above, here are some more ways around this:

  • Somehow require Python 3.10+ for Windows only
  • Leave Issues when running FLORIS 4.1.1 #951 open with a "won't fix" tag so that other users will be able to understand the problem if it comes up (in this case, we'd skip or remove the test)

@paulf81
Copy link
Collaborator Author

paulf81 commented Mar 5, 2025

I prefer dropping 3.9, it's kind of a pain for reasons beyond those in this issue. Sometimes tests crash just for 3.9 and it's because you need to add that from __future__ import annotations bit for 3.9 even though all other versions don't require it.

@RHammond2
Copy link
Collaborator

@rafmudaf, yes, I fully recommend moving to 3.10+. I've already moved a couple projects to 3.10+ at this point because 3.9 is out the door this year anyway, and it was causing some nuisance issues that were hard to resolve. Also, 3.10 introduces match statements, which can be a bit more robust than if/elif/else blocks: https://docs.python.org/3/tutorial/controlflow.html#match-statements and I haven't looked back.

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.

4 participants