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

Set up CI with Azure Pipelines #374

Merged
merged 14 commits into from
Nov 15, 2018
Merged

Set up CI with Azure Pipelines #374

merged 14 commits into from
Nov 15, 2018

Conversation

azure-pipelines[bot]
Copy link
Contributor

@azure-pipelines azure-pipelines bot commented Nov 12, 2018

This PR adds support for Windows and splits plugin tests in multiple jobs. It also addresses a lot of technical debt related to the build in general.

Fixes #354

brettlangdon
brettlangdon previously approved these changes Nov 15, 2018
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

a lot of changes, but seems fairly straight forward.

very minor comments, but nothing blocking this.

- build-node-6
- build-node-8
- build-node-latest
- node-leaks
Copy link
Member

Choose a reason for hiding this comment

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

What we did in Python:

jobs:
  - lint
  - node-leaks
    requires:
      lint

It kind of sucks to setup, but is nice since we don't run all the parallelized jobs until linting passes.

Doesn't happen often though, so... probably didn't save much compute effort.

Copy link
Member

Choose a reason for hiding this comment

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

It's an optimization that I'm thinking of doing mostly if the cost becomes to great. Otherwise it would slow down the entire build since this job would run in sequence.

@@ -77,10 +78,16 @@ class Instrumenter {
}

hookModule (moduleExports, moduleName, moduleBaseDir) {
moduleName = moduleName.replace(new RegExp(`\\${path.sep}`, 'g'), '/')
Copy link
Member

Choose a reason for hiding this comment

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

Would you gain any benefits from defining this RegExp once globally?

@@ -7,6 +7,8 @@ const shimmer = require('shimmer')
const uniq = require('lodash.uniq')
const log = require('./log')

const pathSepExpr = new RegExp(`\\${path.sep}`, 'g')
Copy link
Member

Choose a reason for hiding this comment

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

👍

@rochdev rochdev merged commit 8e9c904 into master Nov 15, 2018
@rochdev rochdev deleted the azure-pipelines branch November 15, 2018 18:34
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.

2 participants