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
Fix tests for Windows and add Azure Pipelines for cross platform CI #5410
Conversation
AppVeyor failing on this PR kinda demonstrates the CI issue here. You can see in the logs that it was expecting a snapshot with CRLF line endings generated from my local Windows box (this test was scoped to only run on Windows machines), but instead it received a snapshot with LF endings generated by AppVeyor. |
Wow, I didn't know that Azure pipeline is free for open source and supports all platform. This looks good to me, AppVeyor is somehow slow and its permission management is weird IMO. (Do people with write access automatically get the permission on Azure pipeline, just like Travis?) @prettier/core what do you think? |
@ikatyang - I am a product manager at Microsoft on Azure Pipelines and can help with these and other questions. Currently Azure Pipelines permissions are managed separately from GitHub's, but we have plans to improve this. Until this ships you would need to add other users to your Pipelines project and give them the necessary permissions. It's pretty easy to add users using their @gmail.com address (or other), but I agree it will be nice when this "just works". Azure Pipelines being free for open source projects (with unlimited build minutes and up to 10 parallel build jobs) was just announced in September, so it's fairly new news. And, yah, having a single CI service build/test across Linux, macOS, and Windows is definitely one of the top reasons other projects are moving to Azure Pipelines. Let us know if we can help with other questions! |
Wow thanks for opening this. This looks strictly better than AppVeyor, so I think we can merge this, and test the Mac/Linux integration for a bit and then change to use only one service? |
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.
How can we enable this for the main repo @damccorm?
@j-f1 it should be pretty straightforward, just need the following steps:
At that point, it should automatically queue builds on PRs and for CI. You can add badges/checks as appropriate. |
@j-f1 - regardless of whether you create a new Azure Pipelines org or @damccorm transfers the https://dev.azure.com/prettier org over (recommended), follow the steps to install the Azure Pipelines app into the prettier GitHub org (or just this repo). This process also steps you through creating the pipeline for your main repo. |
I’ve disabled access to Appveyor in the Prettier settings. Is that enough to stop it from running? |
We probably need to remove |
@ikatyang Done! |
Weird, it's still running. Should we remove the project from AppVeyor? |
I only deleted the webhooks after that commit, so it probably still picked it up.
I don’t want to do that since it would remove the logs from previous builds, which might have value. |
I guess I just joined the org? I can access https://dev.azure.com/prettier now. |
Great! I just made you the org owner, you should be able to create the Pipeline in the prettier project now. |
Should I disable boards and repos for the Azure project? Also, how can I set avatars for the org and project @damccorm? |
@j-f1 disabling boards/repos makes sense to me. Concerning avatars, unfortunately we don't currently support updating avatars for orgs/projects. I agree it would be nice though, hopefully that will change soon. |
Not quite sure how to create the pipeline, I clicked the install our app from the GitHub Marketplace. It navigated me to the GitHub App settings, I clicked Save. There's no prettier org in the list. It seems I have two accounts but I cannot choose which one to use, I'm confused. @j-f1 Are you able to create the pipeline? |
I can’t seem to create it either; any suggestions @damccorm? |
Whoops, I think this was my mistake - I forgot to disconnect the org from the Microsoft user directory. Would you be able to temporarily add me back as an administrator so I can do that? Here are the steps to do that, sorry for the inconvenience.
Note: it may take a few seconds for the list of members to be updated. |
Looks like @ikatyang has to do that. |
I logout and login several times but still got this error: @damccorm I made you the owner again so I guess you should be able to do those things. |
I've disabled AppVeyor GitHub App for this repo, it works (i.e., AppVeyor stopped running). |
Ok, I disconnected it from the Microsoft directory and added @ikatyang back as the project owner, it should be good to go. I'm going to stay in the project as an administrator until its clear this fixed it if that's ok - I'll remove myself once we have a working pipeline. Sorry for the delay on this. |
It works, @damccorm thank you! https://dev.azure.com/prettier/prettier/_build/results?buildId=72&view=logs |
Awesome! I went ahead and removed myself from the org. One additional note, it looks like somewhere in this process the webhooks got messed up so I don't think it will automatically queue builds. Fortunately, the fix for this is really easy, @ikatyang since you created the pipeline just follow this link and click restore: https://dev.azure.com/prettier/prettier/_apps/hub/ms.vss-ciworkflow.build-ci-hub?_a=edit-build-definition&id=5&view=Tab_Triggers Other than that, I think the only remaining steps are:
If you run into any issues/have any Pipelines questions, feel free to ping either me (damccorm@microsoft.com) or Will (wismythe@microsoft.com) or just follow up in this thread. |
Not quite sure why #5491 worked but #5490 did not. 🤔 EDIT: It works on master as well (https://github.com/prettier/prettier/runs/32172122). |
Not 100% sure, but I think it might be because I tried to create a PR with the exact same code I used in this PR which has been merged, so it didn't recognize that it was actually distinct. I think if it worked for #5491 its probably fine, I must have just gotten my wires crossed. We can continue to monitor it, but I think it should be good to go. EDIT: It worked on my PR from a fork in #5492 as well |
It’s been working pretty well from my viewpoint, but @ikatyang probably has more knowledge about this. Thanks for getting us set up! |
It works great! Though there're still some missing features; they're not that critical for us but it'd be better if we could have them. I'm not sure if there's a repo that we can send feature requests to, so I wrote them down here: Missing features:
Feature requests:
|
@ikatyang thanks for the feedback!
I think our goal here is to make sure there's a consistent experience so that status checks aren't jobs for some people and pipelines for others, so in general our guidance is to use one check per pipeline. I'd be happy to provide some guidance on how to do that as well - usually the easiest way is to just add a condition to each job that corresponds to a variable set in the pipeline. So for example, if you want the linting job to be its own check, you could add a seperate "linting pipeline", add the line If that's a change you'd be interested in, let me know, I'd be happy to help! Overall I agree that this adds some complexity versus just allowing job-level status checks, but we're trying to balance that with ensuring a consistent experience. This also allows you to mix and match the number of jobs per check (e.g. you might want to split out windows/mac/Linux checks into separate pipelines but keep the Linux jobs grouped into one check).
This is definitely an area we want to improve in (both caching and I/O in general could be better). A lot of that discussion thus far has centered around private self-hosted agents and bringing your own subscription, but internally I think we're really starting to focus on trying to improve that experience for public repos/hosted agents. We're actively working on this so hopefully we'll see results soon :)
I think you can do this via the Publish Build Artifact task and Download Build Artifact task. So the flow would be you run the build, publish the build artifact, then in the job that depends on the previous job you would download the build artifact.
I haven't done much with codecov, but it looks really useful. It looks like they integrate with a lot of products and it would be cool to see them integrated with Pipelines. As far as I know this isn't something that we're actively working on, but I agree that it would be great to make that happen (though I suspect most of the work to make that happen would be on their side). Regardless, I'd be interested in taking a closer look. |
@damccorm Thanks for the detailed explanation!
I just tried to split out those jobs into multiple pipelines on my fork, it works but there're some confusing things happened:
Summaries from my recent experiments: (pros: ⭕️, cons: ❌) Single pipeline:
Multiple pipelines:
Though I'd love to have multiple status/check, the pros for single pipeline look more important to me. |
Thanks for the heads up, that looks like a bug. I'll file that internally and hopefully we can get that behavior fixed.
I'm not totally sure why this is happening, I haven't seen broken out step templates slow things down, but that is possible.
Agreed, it would be nice to have a view of all the pipeline builds for a commit
That makes sense to me for now, hopefully we can reach a point where some or all of those cons can be reduced/eliminated @willsmythe do you have better ideas for any of the above challenges (or a better way to get multiple status checks)? |
Issue
I recently did a clean clone and tried to run tests, and a bunch of them failed. As I investigated, I realized that it was because I was using a Windows machine that has CRLF default line endings while all of the test snapshots were generated with LF default line endings. Basically there's 2 issues here:
My solution
To address (1), I set the default eol setting to LF in most tests. I also got rid of some recently added tests that rely on the default setting being eol and tried to make platform specific versions of these (see eol-crlf.js and eol-lf.js).
To address (2), I added Azure Pipelines which can natively build/test cross platform. Right now, appVeyor isn't catching this because it uses Unix line endings, but Pipelines runs natively on Windows so it has the default CRLF line endings.
Full disclosure, I work at Microsoft on Pipelines, but I also think its the best solution to this problem - it has big advantages in the form of unlimited minutes, 10 parallel jobs, and the ability to build/test for all platforms with one service.
Additional Info
Here is the pipeline I tested this with on my fork. You can see its failing 36 tests on its first build, those are the tests it was failing before I changed any tests.
After my updates, it passes all tests across all platforms. This replicates all of the current CI in this project.
✨Try the playground for this PR✨