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

GitHub action for 32 bit #37

Closed daniel-mohr closed 2 years ago

daniel-mohr commented 3 years ago

Sorry, initially I was thinking this is a test pull request in my own fork.

As offered in #36 this pull request provides running the test suite on debian latest (32 bit).

Further (as addressed in #31) i combined all test suite run into a single file (but still different jobs).

codecov-commenter commented 3 years ago

Codecov Report

Merging #37 (41b4392) into master (5e819a2) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #37   +/-   ##
=======================================
  Coverage   81.10%   81.10%           
=======================================
  Files           2        2           
  Lines         677      677           
=======================================
  Hits          549      549           
  Misses        128      128

Flag	Coverage Δ
unittests	`81.10% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5e819a2...41b4392. Read the comment docs.

sadielbartholomew commented 3 years ago

Thanks for opening this @daniel-mohr. Sorry we have not got back to you yet about this, there is a lot on at the moment keeping us very busy. In fact, realistically we might not be able to review this until the second half of next working week. I hope that is OK. We will get back to you around then, though. From a quick glance it looks promising!

daniel-mohr commented 3 years ago

That's fine!

By the way: You can also use the docker image i386/debian locally and checkout the version before the fix #32, e. g. git checkout tags/v3.3.3 or git checkout 91e4210, and get:

ERROR: test_Units_conform (test_Units.UnitsTest)
Tests the `conform` class method on `Units`.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/app/cfunits/cfunits/test/test_Units.py", line 151, in test_Units_conform
    x = Units.conform([360], Units("second"), Units("minute"))
  File "/app/cfunits/cfunits/units.py", line 2243, in conform
    y = x.view(dtype=float)
ValueError: When changing to a larger dtype, its size must be a divisor of the total size in bytes of the last axis of the array.

So, #32 is a fix for linux, too. ;-)

And I hope this pull request here (#37) will help to ensure that such things will not happen again in the future.

daniel-mohr commented 2 years ago

Checks on master already not working at the moment. Therefore we have the same errors here.

daniel-mohr commented 2 years ago

Merged #41 here to fix the workflow.

daniel-mohr commented 2 years ago

Yes, I will adapt it.

But I'm not sure windows-latest is the right way. This only runs tests on the 'latest' version and problems like #40 are not detected.

Further at the moment windows-latest is windows-2019 and if windows-2022 becomes stable I expect windows-latest changed to windows-2019 and we would loose testing on windows-2019.

But we could add windows-latest as additional system? (Would lead to an unnecessary run at the moment.)

sadielbartholomew commented 2 years ago

But I'm not sure windows-latest is the right way. This only runs tests on the 'latest' version and problems like #40 are not detected.

Sure, please adapt it however you think is best, so long as we don't use the (soon-to-be) deprecated windows-2016.

daniel-mohr commented 2 years ago

OK, I set windows-2016 as deprecated and added windows-2022 as beta.

So both are used, but the complete workflow should not fail due to these windows versions.