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

ci: enable windows node18 #8164

Closed
wants to merge 1 commit into from

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented May 13, 2022

Description

Since #8076 migrated from jest to vitest, the ci might work.

close #7868
close #8176

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

I think it may be too much to test the three systems in 16 and 18. Previously we were only testing all in a single version (14), and then all the other node versions in ubuntu. I don't recall when this was changed. @Shinigami92 I think you made the PR to add 18? Should we test the three in 16 now and everything else in ubuntu or mac? Or was a discussion that I missed and we want to test the three in 16 and 18 now?

@bluwy
Copy link
Member

bluwy commented May 13, 2022

There's only a brief discussion at #7812 (comment)

@Shinigami92
Copy link
Member

Shinigami92 commented May 13, 2022

I think previously we tested 14, 16 and 17 + 16win and 16mac
Now we test 14, 16 and 18 + 16/18win and 16/18mac
We could remove 16win and 16mac if we feel stable. But as you can explicitly see in this PR #7812, win16 ran green and win18 did not 🤔
But yeah, if this is now stable (and it seems that it now is green 👀) we can remove the other-OS 16 jobs and only leave the 18 ones.
I can do this when I'm not on a 6" display 😛

@sapphi-red
Copy link
Member Author

I ran CI on this PR for five times. It seems it is quite stable. It suceeded every time expect the last time with node18+mac.

@bluwy
Copy link
Member

bluwy commented May 13, 2022

FWIW I'm getting that same CI error when running locally too, and it happens sporadically. Might be a bug in Vitest. (macos 12.3.1, node 16.13.0)

@patak-dev
Copy link
Member

I think we should go back to testing Windows, Mac, and Linux only on Node v16.
I see that @Shinigami92 said we could start doing it also for v18 since we will need it in 6 months, but it seems too soon to me. What about adding that extra cycles for v18 in 3-4 months?

@Shinigami92
Copy link
Member

Yeah, we could do that.
Are we talking about build minutes here? If not, we could just not require win/mac 18 🤷

@patak-dev
Copy link
Member

Yes, I don't think we should use build minutes that don't give us a good return. Our CI is also still a bit flacky (good to see some issues getting squashed after the Vitest switch in that regard though), so that also influences a bit.

@sapphi-red sapphi-red mentioned this pull request May 14, 2022
9 tasks
@sapphi-red
Copy link
Member Author

I agree with patak. Here's the alternative one (#8176).

@sapphi-red sapphi-red deleted the ci/windows-node18 branch May 14, 2022 15:34
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.

4 participants