search

NCAS-CMS / cfunits

A Python interface to UNIDATA’s UDUNITS-2 library with CF extensions:
http://ncas-cms.github.io/cfunits
MIT License
11 stars 8 forks source link

added github actions to run the test suite on windows systems #29

Closed daniel-mohr closed 3 years ago

daniel-mohr commented 3 years ago

Unfortunately the test suite seems to run perfectly on windows using miniconda, but one test test_Units_conform fails. I believe this is due to the new numpy version used (provided by miniconda on windows).

sadielbartholomew commented 3 years ago

Thanks for raising this and putting up a PR, @daniel-mohr. I will review it shortly, by early next working week at the latest.

An initial question I have is why a separate workflow might be required rather than incorporating Windows jobs to the matrix of the existing and similar workflow, run-test-suite.yml? Maybe there is a complication relating to needing to use powershell?

I ask because it would be ideal to avoid duplication of similar jobs and so to include the Windows test in run-test-suite.yml rather than a separate workflow and it might be possible without cluttering up the original workflow much using for example conditional expressions to allow custom steps for the setup required only for Windows.

If it would indeed be possible to incorporate this into the existing workflow in that way, we are happy to review this separate workflow and merge it, then incorporate it into the existing workflow ourselves as follow-on work, but if you are happy to try and incorporate it yourself either as a new commit here or a new PR, that would be great, though not compulsory for us to merge this. Please let us know if you think it could plausibly be done and if so if you'd like to do it or not.

daniel-mohr commented 3 years ago

I did not want to bother to much existing code. Therefore I provided the workflow(s) in different files.

But of cause, it could be combined.

Does it make sense to accept both PRs #29 and #30 first and then combine these 3 files afterwards to one? Or should I create a new PR combining #29 and #30 into run-test-suite.yml?

I would offer to combine it in both cases.

Anyway someone should do this approval of me so that the tests will run.

sadielbartholomew commented 3 years ago

I did not want to bother to much existing code. Therefore I provided the workflow(s) in different files.

Sure, that makes sense.

Does it make sense to accept both PRs #29 and #30 first and then combine these 3 files afterwards to one? Or should I create a new PR combining #29 and #30 into run-test-suite.yml? I would offer to combine it in both cases.

Perfect, thanks again. Whichever approach you prefer is fine.

Anyway someone should do this approval of me so that the tests will run.

Ah yes, good point. I have just approved on both PRs so we can see the results from the CI runs. I can re-trigger if necessary since sometimes we get spurious failures due to coverage API keys failing or similar.

daniel-mohr commented 3 years ago

I think, I would prefer another solution:

Does this makes sense for you?

sadielbartholomew commented 3 years ago

Thanks @daniel-mohr, the approach you have just proposed sounds great. I probably won't have time to look at this until later this evening at the earliest, but I can accept the two open PRs this evening (after a brief review to check I can't see any obvious issues e.g. that the YAML is valid) if that helps you to progress.

daniel-mohr commented 3 years ago

Don't worry, weekend is coming up. I will not do anything before Monday anyway. Take your time!