Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vext01
Copy link
Contributor

@vext01 vext01 commented Jan 30, 2025

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.

@ltratt
Copy link
Contributor

ltratt commented Feb 5, 2025

@vext01 I think we can now consider merging this one?

@vext01
Copy link
Contributor Author

vext01 commented Feb 5, 2025

I think so :)

@ltratt
Copy link
Contributor

ltratt commented Feb 5, 2025

OK, please undraft it?

@vext01 vext01 marked this pull request as ready for review February 5, 2025 15:36
@ltratt ltratt added this pull request to the merge queue Feb 5, 2025
@ltratt ltratt removed this pull request from the merge queue due to a manual request Feb 5, 2025
@ltratt
Copy link
Contributor

ltratt commented Feb 5, 2025

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.

@ltratt ltratt self-assigned this Feb 6, 2025
@vext01
Copy link
Contributor Author

vext01 commented Feb 17, 2025

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?

@ltratt
Copy link
Contributor

ltratt commented Feb 17, 2025

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.

@vext01
Copy link
Contributor Author

vext01 commented Feb 17, 2025

Oops. I was wrong. We test both modes.

If you are happy with that, please merge.

@ltratt
Copy link
Contributor

ltratt commented Feb 17, 2025

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?

@vext01
Copy link
Contributor Author

vext01 commented Feb 17, 2025

I must admit I don't know how long the tests take to run since your changes last week. Will measure.

@vext01
Copy link
Contributor Author

vext01 commented Feb 17, 2025

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?

@ltratt
Copy link
Contributor

ltratt commented Feb 20, 2025

I think this one is ready for squashing?

@vext01
Copy link
Contributor Author

vext01 commented Feb 20, 2025

Currently the extra tests are run only in release mode. With your most recent change, do we want to run in the release-with-assert profile in addition?

@ltratt
Copy link
Contributor

ltratt commented Feb 20, 2025

There is no release-with-assert profile here. It's probably best to run this in debug mode. I suggest doing that but measuring the effect on tryci on b16 (which is currently ~9 minutes): how much does it slow down by?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants