-
Notifications
You must be signed in to change notification settings - Fork 85
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
Update and expand CI jobs #215
Conversation
Think it's fair to separate preprocessing the datafiles since it'll probably not be a shell one-liner. It'll also be good to run |
e931982
to
a6e9c39
Compare
- Disable incremental compilation (useless on CI) - Run clippy first before test jobs - Add `-Dwarnings` to fail on compiler warnings
- Run on macos, linux and windows - Remove explicit build target (host arch is fine)
Manually re-ran test on (ubuntu-latest, nightly) and it passes, not sure where the differences are occurring but it does drive the usage for not using I think it'll take a bit to do an honest review of all tests that use |
I agree. There might be exceptions in some circumstances though (
It would most likely take a fair bit of time, yeah. I'm not sure it's very important to get that done quickly though, for now it should be enough to fix the real errors. Regarding the actual error: The inequality happens at the 17th decimal place. IEEE doubles only really guarantee 15, often 16 decimal places of precision, so this behavior should be perfectly acceptable within IEEE spec. I'll push a fix for it. |
This case caused a test failure on macOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprised it's only one, that's nice. Thanks!
Partially addresses #207. This does not auto-fetch the NIST dataset, I'll explain why below.
This PR:
Example run on my fork (there are quite a few clippy warnings and errors on master, I ""fixed"" these to show the CI run). Looks like one of the nightly tests fails on macOS, maybe because of the ARM architecture.
Often, maintainers don't like merging commits which result in failing CI. So I don't mind having this PR wait until the clippy errors etc. are addressed.
As to why the NIST dataset is not handled (yet): The simple idea was to create a script that downloads the dataset before testing. However, the data on the NIST website is not in the same format as it is in this repo. The text files in this repo are raw line-by-line float values. The actual dataset I found online (link to one of them) links to an ASCII data file that includes a file header and is formatted differently. This would not work without at least a little bit of scripting work, which seems out of scope for this PR.