-
Notifications
You must be signed in to change notification settings - Fork 228
Add missing headers so that all headers compile independently #1154
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
Conversation
|
Is this newly added test very heavy? All of the files in the library are compiled. AFAIR we decided not to run it automatically because it takes a lot of time. How long does it take vs minimal and documentation tests? |
|
Good point. I think it was not to run automatically because it was failing (see #525 (comment)). Also this PR does not change that, it still do not run automatically but only called by the new github action. Regarding performance I got the following timings (on my laptop with
Note that for the minimal test the timing refers to testing only 1 out of 14 compilers. So roughly speaking the headers test is the 1/3 of the current tests run on actions (i.e. minimal & documentation). It is somehow heavy I agree. Alternatively, we can enable it only for push and not for PRs or even add it to circleCI (where the percentage over the total test time would be much smaller). |
|
@awulkiew the new test has picked up by github actions so now you can also check the times in the server; i.e. 20m for |
barendgehrels
left a comment
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.
Thanks! I didn't review all changes in detail, but in general it looks good to me.
|
Right, so it doesn't take that long vs all of the tests, since it's done only once. Thanks! |
Fixes #523
Also adds a github actions script to test that all headers compile independently.