Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Azure pipelines #838

Merged
merged 4 commits into from
Mar 16, 2019
Merged

Azure pipelines #838

merged 4 commits into from
Mar 16, 2019

Conversation

apawast
Copy link
Contributor

@apawast apawast commented Mar 6, 2019

PR checklist

Overview of change:

Add support for CI using Azure Pipelines

Is there anything you'd like reviewers to focus on?

I have created a build pipeline for tslint-microsoft-contrib project here: https://dev.azure.com/ms/tslint-microsoft-contrib
Microsoft employees should all have access to this already. Currently it's building off of my fork of the project, however, you or Will Smythe can change it to point to your repo.

I had to add the reporter to publish the results:

tests

However, I found a new solution so that the reporter will not affect developers working on this project.

@apawast
Copy link
Contributor Author

apawast commented Mar 6, 2019

@JoshuaKGoldberg

package.json Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link

Whoops, I accidentally pushed a commit to your azure-pipelines branch. Sorry about that 😄 force-pushed it out of the history.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Well this is spooky. Here are three PRs with failing tests:

The .ts.lint test correctly fails, but neither build with failing Mocha tests picked up on it. That seems like a blocker (unless there was user error on my end?).

@apawast
Copy link
Contributor Author

apawast commented Mar 7, 2019

Well this is spooky. Here are three PRs with failing tests:

  • apawast#1: failing .ts.lint test; correctly reports the failure
  • apawast#2: failing Mocha test with the cross-var changes
  • apawast#3: failing Mocha test without any package changes

The .ts.lint test correctly fails, but neither build with failing Mocha tests picked up on it. That seems like a blocker (unless there was user error on my end?).

I reproduced those test failure locally and realized that the message that you changed isn't what it's testing for, it's testing for true or false. The message could be anything and it'll pass regardless. I changed the tests to say false instead of true and it failed locally immediately. I didn't want to have random failing builds in the pipeline setup so I didn't push but if you want I can reproduce it in a different branch or something.

Also thanks for looking into this!

@JoshuaKGoldberg
Copy link

Ha, user error on my end indeed. I'll take another look. Thanks!

@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! label Mar 10, 2019
@JoshuaKGoldberg
Copy link

JoshuaKGoldberg commented Mar 10, 2019

@apawast apawast#3 is correctly on failed unit tests after reverting changes to package.json. That seems to indicate changes to package.json in this PR can be reverted, right? Unless I'm (again) missing something, could you please?

Also, in case it's useful, some feedback about Azure Pipelines itself:

I'm happy to send you / your team more detailed feedback here or via another channel if you'd like.

@JoshuaKGoldberg JoshuaKGoldberg added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Mar 10, 2019
@apawast
Copy link
Contributor Author

apawast commented Mar 12, 2019

@JoshuaKGoldberg The changes I made to package.json are for test reporting. Azure Pipelines also has rich test reporting which works well with commonly used test runners including the mocha-junit-reporter. If you switch from the Logs tab to the Tests tab of a build you can see the test reporting. The default setting of the pipeline is to show failed/aborted tests so if a test failed, you wouldn't have to look through the logs, you could just view them in the Test tab first thing (which helps with the colorless logs issue). However, I have also added a force color variable to ensure the logs show colored text too.

For the issue about getting through several clicks, that is just how Github handles getting to the pipeline from the repo. We can't do anything about it, however, I have heard there is a project in the works to fix this.

Any other feedback you have, you can email it to me at [email protected] and I will be happy to pass it on to the appropriate people!

@JoshuaKGoldberg
Copy link

JoshuaKGoldberg commented Mar 13, 2019

The changes I made to package.json are for test reporting. Azure Pipelines also has rich test reporting which works well with commonly used test runners including the mocha-junit-reporter

Yes, but tslint and tslint-microsoft-contrib don't use a commonly used test runner. From #655:

Also worth to add: there are new npm tasks that should generate report from Mocha. However #489 will migrate almost all test away from Mocha, and only test for few utils and deprecated formatters will left as mocha tests. I do not think that it worth it to have report just for them.

For this PR to be merged, there should be no package.json or package-lock.json changes.

I have also added a force color variable

Awesome, thanks!

that is just how Github handles getting to the pipeline from the repo. We can't do anything about it

No, it's how you handle getting to the pipeline from the repo 😄. See this PR on CircleCI: budgielang/budgie#641. Clicking Details takes you directly to a well-formatted display of which tasks failed.

Anyway, this is bikeshedding - the real sticking point here is the package.json changeset.

@apawast
Copy link
Contributor Author

apawast commented Mar 13, 2019

Okay I see. I have no problems removing the changes from them if you're okay with no test reporting (at least the logs will be colorful and I'm sure that's what you're used to looking at them anyways).

I don't really know much about that repo to pipelines process, I was just relaying what other PMs informed me. But I'll definitely look into it!

@apawast
Copy link
Contributor Author

apawast commented Mar 13, 2019

@JoshuaKGoldberg I changed them to match your master. Everything look okay?

@JoshuaKGoldberg JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Mar 16, 2019
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🙌

@JoshuaKGoldberg
Copy link

Node 6 build failures are unrelated.

@JoshuaKGoldberg JoshuaKGoldberg merged commit fc41f7c into microsoft:master Mar 16, 2019
@JoshuaKGoldberg
Copy link

Thanks for setting this up @apawast! We'll try this out for a while and send you feedback as we come across it.

@JoshuaKGoldberg
Copy link

Oh, just a heads up, I don't have permissions to add it as an app to this repository. You'll want to work with @HamletDRC for that, I believe. It might need an admin who's a member of the @microsoft organization.

@JoshuaKGoldberg JoshuaKGoldberg added this to the None milestone Apr 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Build, Test and Release tslint-microsoft-contrib using Azure Pipelines
2 participants