-
Notifications
You must be signed in to change notification settings - Fork 7
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 third party yklua tests to CI. #1575
base: master
Are you sure you want to change the base?
Conversation
@vext01 I think we can now consider merging this one? |
I think so :) |
OK, please undraft it? |
I've removed these from the queue because these have so far spent an hour running in CI, which is clearly vastly too long for us to want to run for each PR. |
Re-visiting this, just noticed that we only test yklua with serialised compilation. I followed precedent in this PR, but is that what we want? |
Serialised compilation is a choice with pros and cons. In general it forces more things to be tested, but "in general" is the key qualifier. |
Oops. I was wrong. We test both modes. If you are happy with that, please merge. |
The reason I cancelled the merge is that these tests took over an hour to run. We can't merge them until we know they run in reasonable time. It might be that these long-running tests should be run overnight, or after a PR's merged, or something like that? |
I must admit I don't know how long the tests take to run since your changes last week. Will measure. |
I've merged master and added a commit that runs the extra tests only in release mode in the interest of saving time. That should be an OK compromise. One of the tests hits the ">32-bit constant" todo that is fixed as part of #1606, so we will need to wait for this first. OK to squash in the meantime? |
I think this one is ready for squashing? |
Currently the extra tests are run only in release mode. With your most recent change, do we want to run in the |
There is no |
This currently fails due to this deopt bug, hence this is a draft.
A similar change should be applied to
yklua
so that these tests are run in CI there too.I'll start investigating that deopt bug tomorrow.