Using DangerJS to check changes coupling of implementation files to test or documentation files #2523
Conversation
Dangerfile.js
Outdated
There was a problem hiding this comment.
Can we change this to TS!
There was a problem hiding this comment.
Good point, I will work on it.
Dangerfile.js
Outdated
| const allModifiedFiles = [...git.modified_files, ...git.created_files]; | ||
| const messages = []; | ||
|
|
||
| Object.entries(couplings).forEach(([implFile, testOrDocFiles]) => { |
There was a problem hiding this comment.
This makes sense to me in theory, but given that the sample couplings you have here are package.json and either lerna or babel, which is not a TestOrDoc file, I think reading this code is confusing. Maybe we should name it as "dependent files" or something?
Or maybe this isn't such a good example of a coupling? You could put site and site test maybe 🤔
KevinEyo1
force-pushed
the
2140-file-diff
branch
from
101c546
to
f448b73
Compare
There was a problem hiding this comment.
changes look good - added a nit about how it should be logged.
i am concerned about people not checking console logs for CI, its a lot more troublesome and honestly wouldn't occur to me. But since this is a piece by piece implementation and you've mentioned you'd like to look at generating an automated list of reliant files, I think it is OK for now
EDIT: Also tests are failing - do fix thanks
dangerfile.ts
Outdated
| }); | ||
|
|
||
| if (messages.length > 0) { | ||
| logger.info(`Detected issues with file couplings:\n${messages.join('\n')}`); |
There was a problem hiding this comment.
maybe log it as a warning?
dangerfile.ts
Outdated
| 'packages/core/src/html/CustomListIconProcessor.ts': [ | ||
| 'docs/userGuide/syntax/lists.md', | ||
| 'packages/cli/test/functional/test_site/testList.md', | ||
| 'packages/cli/test/functional/test_site/expected/testList.html', |
There was a problem hiding this comment.
Hmmm tbh i feel like there isn't a need to add the .html and .page-vue-render.js files here. Because they will have to updated otherwise the tests will fail automatically.
|
@KevinEyo1 Each PR must have a SEMVER impact label, please remember to label the PR properly. |
What is the purpose of this pull request?
Overview of changes:
#2140
Added a workflow that utilizes DangerJS to check that certain implementation files having changes, has changes in its corresponding test or documentation files.
Anything you'd like to highlight/discuss:
Currently run on
pull_requestand logs the error in CI logs. Does not include implementation of manual/automatic coupling of files. Once that is implemented with good accuracy, workflow can change to failing the check instead of just logging the error.Testing instructions:
Based on test couplings, proper error logs should be found in CI logs
Proposed commit message: (wrap lines at 72 characters)
It can be easy to miss out on updating test or documentation files
when implementing or changing features.
Let's add a workflow utilizing DangerJS to aid in checking and
alerting developers if they forget to do so.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):