danger.git.modified_files contains undefined
- Dominant language
- TypeScript
- Stars
- 5.5k
- Forks
- 381
- Avg merge
- 2h 8m
- Merged PRs (30d)
- 6
Description
It looks like `danger.git.modified_files` (and potentially `deleted_files` or `created_files`?) can include `undefined` as a filename.
The bug was discovered in a private repo at work, so I can't share links / haven't been able to dig deeply into the cause yet, but I did log out the DSL [right before `danger.git` is created](https://github.com/danger/danger-js/blob/a6223c12b6a84e9308727745768d4c89baad3573/source/platforms/github/GitHubGit.ts#L169):
```
{ modifiedDiffs:
[ { chunks: [Object],
deletions: 1,
additions: 1,
from: 'dangerfile.ts',
to: 'dangerfile.ts',
index: [Object] },
{ chunks: [Object],
deletions: 8,
additions: 32,
from: 'sql/[redacted].sql',
to: 'sql/[redacted].sql',
index: [Object] },
{ chunks: [],
deletions: 1,
additions: 5,
from: '[redacted content from sql/[redacted].sql]' } ],
addedDiffs: [],
removedDiffs: [] }
```
There `danger.git.modified_files` equals:
```
[ 'dangerfile.ts', 'sql/[redacted].sql', undefined ]
```
Would it make sense to filter out any falsy values before returning `danger.git.(modified|created|deleted)_files`? It seems like a band aid solution, but it also was unexpected to try to execute the following code and seeing Danger fail:
```ts
const testFiles = danger.git.modified_files
.concat(danger.git.created_files)
// `path` may be undefined here
.filter(path => path.endsWith('.test.ts'))
```
Would love suggestions on preventing `undefined` from sneaking through. Will try to get redacted GitHub API data from my work repo and push up a failing test too. 🙂
Contributor guide
Research direction
Start at source/platforms/github/GitHubGit.ts around the point where the DSL is logged before danger.git is created. Inspect how modifiedDiffs, addedDiffs, and removedDiffs become the modified_files, created_files, and deleted_files arrays, then reproduce the reported undefined filename with representative diff data. Done means the file lists no longer contain undefined values and a regression test covers the case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github, typescript
- Domain
- api
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100