Provide primitives for "if this file exists, then so should this" sort of rules

[orta]

Basically, provide some kind of way to simplify rules like these

// Check that every file touched has a corresponding test file
const correspondingTestsForModifiedFiles = modifiedFiles.map(f => {
  const newPath = path.dirname(f);
  const name = path.basename(f);
  return `${newPath}/__tests__/${name}`;
});

const testFilesThatDontExist = correspondingTestsForModifiedFiles
  .filter(f => !f.includes('__stories__')) // skip stories
  .filter(f => !fs.existsSync(f));

if (testFilesThatDontExist.length > 0 && !skipTests) {
  const output = `Missing Test Files:
${testFilesThatDontExist.map(f => `- \`${f}\``).join('\n')}
If these files are supposed to not exist, please update your PR body to include "Skip New Tests".`;
  warn(output);
}

// Find changed React components
const changedComponents = modifiedFiles.filter(file => {
  const content = fs.readFileSync(file).toString();
  return content.includes('React');
});

// Check for images in PR description if new components added
if (changedComponents.length > 0 && !skipVisualDiff && !hasScreenShots) {
  const output = `Should there be images to represent these components:
  ${changedComponents.map(f => `- \`${f}\``).join('\n')}
  If these changes are not visual, please update your PR body to include "Skip Visual Diff".`;
  warn(output);
}

// Check that every component touched has a corresponding story
const correspondingStoriesForChangedComponents = uniq(
  changedComponents.map(f => {
    const newPath = path.dirname(f);
    return `${newPath}/__stories__/index.js`;
  })
);

const missingStories = correspondingStoriesForChangedComponents.filter(
  f => !fs.existsSync(f)
);

and

// Custom modifiers for people submitting PRs to be able to say "skip this"
const trivialPR = bodyAndTitle.includes("trivial")
const acceptedNoTests = bodyAndTitle.includes("skip new tests")

const typescriptOnly = (file: string) => includes(file, ".ts")
const filesOnly = (file: string) => fs.existsSync(file) && fs.lstatSync(file).isFile()

// Custom subsets of known files
const modifiedAppFiles = modified.filter(p => includes(p, "lib/")).filter(p => filesOnly(p) && typescriptOnly(p))

// Modified or Created can be treated the same a lot of the time
const touchedFiles = modified.concat(danger.git.created_files).filter(p => filesOnly(p))

const touchedAppOnlyFiles = touchedFiles.filter(
  p => includes(p, "src/lib/") && !includes(p, "__tests__") && typescriptOnly(p)
)

const touchedComponents = touchedFiles.filter(p => includes(p, "src/lib/components") && !includes(p, "__tests__"))

// Rules
// When there are app-changes and it's not a PR marked as trivial, expect
// there to be CHANGELOG changes.
const changelogChanges = includes(modified, "CHANGELOG.md")
if (modifiedAppFiles.length > 0 && !trivialPR && !changelogChanges) {
  fail("No CHANGELOG added.")
}

// Check that every file touched has a corresponding test file
const correspondingTestsForAppFiles = touchedAppOnlyFiles.map(f => {
  const newPath = path.dirname(f)
  const name = path.basename(f).replace(".ts", "-tests.ts")
  return `${newPath}/__tests__/${name}`
})

// New app files should get new test files
// Allow warning instead of failing if you say "Skip New Tests" inside the body, make it explicit.
const testFilesThatDontExist = correspondingTestsForAppFiles
  .filter(f => !f.includes("Index-tests.tsx")) // skip indexes
  .filter(f => !f.includes("types-tests.ts")) // skip type definitions
  .filter(f => !f.includes("__stories__")) // skip stories
  .filter(f => !f.includes("AppRegistry")) // skip registry, kinda untestable
  .filter(f => !f.includes("Routes")) // skip routes, kinda untestable
  .filter(f => !f.includes("NativeModules")) // skip modules that are native, they are untestable
  .filter(f => !f.includes("lib/relay/")) // skip modules that are native, they are untestable
  .filter(f => !f.includes("Storybooks/")) // skip modules that are native, they are untestable
  .filter(f => !fs.existsSync(f))

if (testFilesThatDontExist.length > 0) {
  const callout = acceptedNoTests ? warn : fail
  const output = `Missing Test Files:
${testFilesThatDontExist.map(f => `- \`${f}\``).join("\n")}
If these files are supposed to not exist, please update your PR body to include "Skip New Tests".`
  callout(output)
}

[paschalidi]

@orta hey keep the nice work up 🎉 . That sounds like a cool and useful feature to have indeed.
I am happy to spent time on that. I will be busy for a month but after 15th of April I can start working on it if nobody else does so.
Do you think there is some code that I can look into that would do something "similar"?

[orta]

I think maybe the right way to address this is to figure out what the API someone would use in their Dangerfile is. That will probably strongly influence your implementation details.

So, maybe first we try see what it takes to make a set of primitives for doing this?

[paschalidi]

@orta 👋 and ☮️. Good day!

I have put some thought into this.

1. We would need some sort of a configuration file.
   For me there are two kinds of projects when it comes to testing.
   Some people like to have the tests close to their js-files/components and some others like to have all the tests in a separate folder that would mirror the actual src. I would guess that we would like to take into account both. Can you please confirm tho❔

2. Let the previous aside. We want to be able to use danger by running a command on the terminal that will help you configure and set some of the primitives on or you have a better idea of how this would look like❔

3. Is the idea that danger be in the marketplace as a solution that will help you have consistent PRs❔

Thanks for taking the time to make this possible for all of the community. Happy that I can be part of this :)

[orta]

No I think you're a bit off base , Danger doesn't provide rules - Danger provides tools to make rules. This issue is about providing another API function which does a good job of making it easy to do file checking.

An example of resolving this issue is chainsmoker by @paulmelnikow

Perhaps Danger could depend on it, and expose the chainsmoker as default export:

import {danger} from "danger"

// danger.git.match = chainsmoker({
//   created: danger.git.created_files,
//   modified: danger.git.modified_files,
//   createdOrModified: danger.git.modified_files.concat(danger.git.created_files),
//   deleted: danger.git.deleted_files,
// })

const documentation = danger.git.match(
  '**/*.md',
  'lib/all-badge-examples.js',
  'frontend/components/usage.js'
)
const packageJson = danger.git.match('package.json')
const packageLock = danger.git.match('package-lock.json')
const helpers = danger.git.match('lib/**/*.js', '!**.spec.js')
const helperTests = danger.git.match('lib/**/*.spec.js')

// This is `true` whenever there are matches in the corresponding path array.
if (documentation.createdOrModified) {
  message('We :heart: our [documentarians](http://www.writethedocs.org/)!')
}

if (packageJson.modified && !packageLock.modified) {
  warn('This PR modified package.json, but not package-lock.json')
}

if (helpers.created && !helperTests.created) {
  warn('This PR added helper modules in lib/ but not accompanying tests.')
} else if (helpers.createdOrModified && !helperTests.createdOrModified) {
  warn('This PR modified helper modules in lib/ but not accompanying tests.')
}

[paschalidi]

@orta thanks for taking the time to explain. That makes more sense now, I will start working on that and open a PR for people to review.

☮️ thanks again 👍

[orta]

No worries!

/cc @paulmelnikow - maybe we could vendor in (and TypeScript-erize chainsmoker), we could make minimatch a danger dep, then it reduces the dependency tree for users?

[paulmelnikow]

Sure! I'd love to have this included in Danger.

I'm inclined to keep chainsmoker installable from npm in case folks want to use it apart from Danger, though if you don't want to make it a dependency (it's 50 lines after all) why don't we do as you're suggesting, and then I'll sync the changes back once it stabilizes. We could switch to micromatch; it does the same thing and it's smaller and faster.

I'd be happy to do this work! Let me know how you want to handle it.

[orta]

Micromatch is a good call, because jest uses it - and there's probably a big overlap in Jest and Danger users.

My preference is always to keep the Danger dep tree matching with Jest when I have any choices to make.

OK, so, let's vendor in the micromatch index.js into danger somewhere, add it to the git DSL - if you get it working in a PR, I can handle all of the documentation side of it and wrap it up