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

Fix tests for Windows and add Azure Pipelines for cross platform CI #5410

Merged
merged 10 commits into from Nov 14, 2018
Merged

Fix tests for Windows and add Azure Pipelines for cross platform CI #5410

merged 10 commits into from Nov 14, 2018

Conversation

damccorm
Copy link
Contributor

@damccorm damccorm commented Nov 8, 2018

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:

  1. Tests are failing for Windows because of line endings. This is especially alarming because it means that line endings weren't really being tested in the way we thought they were, a topic which has been a significant development area recently: see fix(html): treat CRLF as LF #5393 and Add option to enforce certain line endings #5327 for reference
  2. These weren't caught by the current CI

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.
image

After my updates, it passes all tests across all platforms. This replicates all of the current CI in this project.
image

Try the playground for this PR

@damccorm
Copy link
Contributor Author

damccorm commented Nov 8, 2018

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.

@ikatyang
Copy link
Member

ikatyang commented Nov 9, 2018

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?

@willsmythe
Copy link

@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!

@duailibe
Copy link
Member

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?

Copy link
Member

@j-f1 j-f1 left a 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?

@damccorm
Copy link
Contributor Author

@j-f1 it should be pretty straightforward, just need the following steps:

  1. Merge this PR in. You probably want to disable AppVeyor here too since it fails some of the tests I added.
  2. I'll transfer access of the pipelines organization I've been using to you. You can also create your own, but I figured you'd like the org name Prettier.
  3. You can create a pipeline pointing at master of this repo. To do this, just go to the Pipelines tab, click New->New Build Pipeline and follow the prompts. It should automatically detect the Yaml from this PR, so no additional work should be needed there.

At that point, it should automatically queue builds on PRs and for CI. You can add badges/checks as appropriate.

@willsmythe
Copy link

@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.

@j-f1
Copy link
Member

j-f1 commented Nov 14, 2018

I’ve disabled access to Appveyor in the Prettier settings. Is that enough to stop it from running?

@j-f1 j-f1 merged commit 18b03a3 into prettier:master Nov 14, 2018
@ikatyang
Copy link
Member

We probably need to remove .appveyor.yml and its webhook as well.

j-f1 added a commit that referenced this pull request Nov 14, 2018
@j-f1
Copy link
Member

j-f1 commented Nov 14, 2018

@ikatyang Done!

@ikatyang
Copy link
Member

Weird, it's still running. Should we remove the project from AppVeyor?

@damccorm
Copy link
Contributor Author

@j-f1 @ikatyang @duailibe I just sent you email invitations to join the org as administrators. Once one of you confirms access I'll transfer ownership of the org.

@j-f1
Copy link
Member

j-f1 commented Nov 14, 2018

I only deleted the webhooks after that commit, so it probably still picked it up.

Should we remove the project from AppVeyor?

I don’t want to do that since it would remove the logs from previous builds, which might have value.

@ikatyang
Copy link
Member

I guess I just joined the org? I can access https://dev.azure.com/prettier now.

@damccorm
Copy link
Contributor Author

Great! I just made you the org owner, you should be able to create the Pipeline in the prettier project now.

@j-f1
Copy link
Member

j-f1 commented Nov 14, 2018

Should I disable boards and repos for the Azure project? Also, how can I set avatars for the org and project @damccorm?

@damccorm
Copy link
Contributor Author

@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.

@ikatyang
Copy link
Member

Not quite sure how to create the pipeline, I clicked the install our app from the GitHub Marketplace.

image: install our app from the GitHub Marketplace

image


It navigated me to the GitHub App settings, I clicked Save.

image: GitHub App settings - save

image


There's no prettier org in the list.

image: setup your azure pipelines project

image


It seems I have two accounts but I cannot choose which one to use, I'm confused.

image: two account

image


@j-f1 Are you able to create the pipeline?

@j-f1
Copy link
Member

j-f1 commented Nov 14, 2018

I can’t seem to create it either; any suggestions @damccorm?

@damccorm
Copy link
Contributor Author

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.

  1. Go to https://dev.azure.com/prettier/_settings/security?_a=members
  2. Select the "Project Collection Administrators" group and then select the "Members" tab
  3. Click Add, enter "damccorm@microsoft.com", click Save changes

Note: it may take a few seconds for the list of members to be updated.

@j-f1
Copy link
Member

j-f1 commented Nov 14, 2018

Looks like @ikatyang has to do that.

@ikatyang
Copy link
Member

ikatyang commented Nov 15, 2018

I logout and login several times but still got this error:

image

@damccorm I made you the owner again so I guess you should be able to do those things.

@ikatyang
Copy link
Member

I've disabled AppVeyor GitHub App for this repo, it works (i.e., AppVeyor stopped running).

@damccorm
Copy link
Contributor Author

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.

@ikatyang
Copy link
Member

It works, @damccorm thank you!

https://dev.azure.com/prettier/prettier/_build/results?buildId=72&view=logs

@damccorm
Copy link
Contributor Author

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:

  1. Add Azure Pipelines as a check for the PR
  2. (optionally) Add a badge to the Readme
  3. (optionally) Switch to just use one service once Pipelines is confirmed to be working for a while

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.

@ikatyang
Copy link
Member

It seems we don't need to restore? I didn't see it.

image

@ikatyang
Copy link
Member

ikatyang commented Nov 15, 2018

Not quite sure why #5491 worked but #5490 did not. 🤔

EDIT: It works on master as well (https://github.com/prettier/prettier/runs/32172122).

@damccorm
Copy link
Contributor Author

damccorm commented Nov 15, 2018

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

@damccorm
Copy link
Contributor Author

@ikatyang @j-f1 @duailibe just wanted to follow up and see how things are going with Pipelines! Do you feel like its working as well as CircleCI and Travis/have you run into any problems with it that I could help out with?

@j-f1
Copy link
Member

j-f1 commented Nov 29, 2018

It’s been working pretty well from my viewpoint, but @ikatyang probably has more knowledge about this. Thanks for getting us set up!

@ikatyang
Copy link
Member

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:

  • one status/check per job (it's possible to use one status/check per pipeline but it looks overcomplicated to me.)
    image
  • Azure Pipelines does not support cache, though it's fast enough that the build time is still acceptable, it'd be better to be faster.
  • CircleCI can store artifacts in the previous job and pass them to the next job. From what I understand from the docs on Azure Pipelines, it seems it only supports executing job depends on the previous job, but the artifacts can not be passed to the next job.
    image

Feature requests:

  • I saw Azure Pipelines also supports coverage, and somebody even built a badge for it (Azure DevOps coverage on https://shields.io/). I wonder if it's possible to have something like codecov in Azure Pipelines.
    image

@damccorm
Copy link
Contributor Author

@ikatyang thanks for the feedback!

one status/check per job (it's possible to use one status/check per pipeline but it looks overcomplicated to me.)

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 condition: variables.linting_job to the YAML file, and only set that true in the linting pipeline. From there you'd just need to update repo settings -> branches -> require status checks to include the linting pipeline.

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).

Azure Pipelines does not support cache, though it's fast enough that the build time is still acceptable, it'd be better to be faster.

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 :)

CircleCI can store artifacts in the previous job and pass them to the next job. From what I understand from the docs on Azure Pipelines, it seems it only supports executing job depends on the previous job, but the artifacts can not be passed to the next job.

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 saw Azure Pipelines also supports coverage, and somebody even built a badge for it (Azure DevOps coverage on https://shields.io/). I wonder if it's possible to have something like codecov in Azure Pipelines.

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.

@ikatyang
Copy link
Member

ikatyang commented Dec 6, 2018

@damccorm Thanks for the detailed explanation!

You could add a seperate "linting pipeline", add the line condition: variables.linting_job to the YAML file, and only set that true in the linting pipeline.

I just tried to split out those jobs into multiple pipelines on my fork, it works but there're some confusing things happened:

  • some pipelines are still marked as pending on GitHub even if it's passed on Azure Pipelines:

    image: commit status on my fork (url)
  • the start time of the last pipeline is slower than the first pipeline about 2 min.

    • this may be caused by the fact that I created a lot of reusable step templates, I wonder if it's possible to have inline templates to reduce the I/O time.
  • it's hard to navigate the same commit between different pipelines on the Azure Pipelines website, we need to:

    1. go back to the Builds page
    2. select the pipeline
    3. select the commit (this can be hard if there're a lot of different PR builds)
    4. select the job
      (and also the jobs view here looks confusing since there're a lot of skipped)

    comparing to the current single pipeline approach:

    1. select the job
    2. done!

Summaries from my recent experiments: (pros: ⭕️, cons: ❌)

Single pipeline:

  • ❌only one status/check
  • ❌only one badge
  • ⭕️navigation between jobs is easy
  • ⭕️no need to configure on the Azure Pipelines website, everything is controlled by the yaml config
  • ⭕️no need to think about the pipeline start time

Multiple pipelines:

  • ⭕️can have multiple status/check, we can observe the entire status at a glance
  • ⭕️can have multiple badges (e.g., one for Windows, one for Linux, etc.)
  • ❌navigation between jobs (across pipelines) can be hard
  • ❌need to configure on the Azure Pipelines website, which means it's a global setting and it's not possible to only enable it in the specific PR (for experiments)
  • ❌the pipeline start time may be slow

Though I'd love to have multiple status/check, the pros for single pipeline look more important to me.

@damccorm
Copy link
Contributor Author

damccorm commented Dec 6, 2018

some pipelines are still marked as pending on GitHub even if it's passed on Azure Pipelines

Thanks for the heads up, that looks like a bug. I'll file that internally and hopefully we can get that behavior fixed.

the start time of the last pipeline is slower than the first pipeline about 2 min. this may be caused by the fact that I created a lot of reusable step templates, I wonder if it's possible to have inline templates to reduce the I/O time.

I'm not totally sure why this is happening, I haven't seen broken out step templates slow things down, but that is possible.

It's hard to navigate the same commit between different pipelines on the Azure Pipelines website, we need to

Agreed, it would be nice to have a view of all the pipeline builds for a commit

Though I'd love to have multiple status/check, the pros for single pipeline look more important to me.

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)?

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Mar 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants