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

Move visual studio tests from per-release to per-PR #2845

Merged
merged 2 commits into from
Nov 30, 2021

Conversation

senhuang42
Copy link
Contributor

@senhuang42 senhuang42 commented Nov 3, 2021

Tests for visual studio 2008, 2010, 2012, and 2013 on appveyor were running only on the release branch. Github Actions only supports Visual Studio 2015 and beyond, so we have to preserve these tests.

This moves over the tests to run on branches against dev, and removes 2008 and 2010 tests, since those would exceed the previously discussed 10-year time window for compiler support.

Note that these Visual Studio build tests seem to complain more than the Github Actions ones, picking up on an unused local function issue, since Visual Studio doesn't have an equivalent of __attribute__(unused).

This should get merged after #2846

@senhuang42 senhuang42 marked this pull request as ready for review November 3, 2021 14:11
@senhuang42 senhuang42 changed the title msvc tests to dev Move visual studio tests from per-release to per-PR Nov 3, 2021
@Cyan4973
Copy link
Contributor

Cyan4973 commented Nov 3, 2021

Let's see after #2846 .
We don't want to inflate test duration on appveyor too much.
We may have to settle on a "good enough" middle ground to avoid making this task an annoying CI bottleneck.

@senhuang42
Copy link
Contributor Author

I've reduced the length of the tests, and also removed the VS 2013 test, so now we only have a VS 2012 test (having both seems redundant). So now, we test VS 2012, 2015, and 2019 - which seems like good enough coverage.

Looking at appveyor.yml, I realize that the github actions tests don't perform some of these basic fuzz tests. In a follow-up, I'll add that for the VS 2015, 2019 tests as well.

@senhuang42 senhuang42 force-pushed the appveyor_msvc2 branch 2 times, most recently from 420191b to 6d9f6c6 Compare November 9, 2021 21:07
@senhuang42 senhuang42 force-pushed the appveyor_msvc2 branch 5 times, most recently from d5bc812 to 45814df Compare November 24, 2021 18:08
@Cyan4973
Copy link
Contributor

It seems I don't see any Appveyor test running ?

@senhuang42
Copy link
Contributor Author

It seems I don't see any Appveyor test running ?

Not sure what happened there, but I re-force pushed and it seems to have run and passed this time.

@senhuang42
Copy link
Contributor Author

senhuang42 commented Nov 29, 2021

Though, I will say that the Release builds take 15-20mins each to build, which is about 10x as long as the GH Actions release builds take. But there's no way to migrate these off of Appveyor, since GH Actions doesn't have MSVC 2012. The options appear to be 1) eat this 40m appveyor cost or 2) Drop the 2012 release builds entirely as not much seems to still support MSVC 2012. (and moving these 2012 release builds to per-release brings us back to the original issue, so it's a non-option).

@Cyan4973
Copy link
Contributor

Indeed, Appveyor tests are now at 38mn (!),
making them by far the slowest element of CI.

In details, I notice that debug visual tests cost only ~1mn each, which is fine,
while release tests are 15-20mn each,
suggesting a difference in scope.

Maybe we should reduce the scope of release tests, or even consider removing them entirely.
I presume debug tests already cover build issues,
which means release tests are mostly present for runtime issues.
Do we need those ?

@senhuang42
Copy link
Contributor Author

Maybe we should reduce the scope of release tests, or even consider removing them entirely. I presume debug tests already cover build issues, which means release tests are mostly present for runtime issues. Do we need those ?

It's probably good to still have a few runtime tests for windows. It will be better to remove these from Appveyor, and add a new long GH actions test that runs the useful ones (some of these tests look not very useful).

@senhuang42 senhuang42 force-pushed the appveyor_msvc2 branch 6 times, most recently from e894529 to a6af0ca Compare November 29, 2021 20:42
Copy link
Contributor

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With latest decrease to test durations, with Release runtime tests in the ~2mn range, I believe this PR looks in good enough shape.

@senhuang42 senhuang42 merged commit 9bc94b2 into facebook:dev Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants