From 0c82b00c97ba7665e086edb16e08788ec203c9ed Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Fri, 15 Mar 2024 23:23:56 +0100 Subject: [PATCH] Improve error message when failing to update a suppression file --- CHANGELOG.md | 2 + lib/suppressed-errors.js | 19 +++++- .../elm.json | 24 ++++++++ .../expected-NoUnused.Variables.json | 8 +++ .../fixed-elm.json | 24 ++++++++ .../review/elm.json | 34 +++++++++++ .../review/src/ReviewConfig.elm | 24 ++++++++ .../review/suppressed/NoUnused.Variables.json | 9 +++ .../src/Main.elm | 58 +++++++++++++++++++ .../src/OtherFile.elm | 5 ++ .../with-errors-OtherFile.elm | 13 +++++ test/snapshots/suppress/write-failure.txt | 8 +++ test/suppress.test.js | 17 ++++-- 13 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 test/project-with-suppressed-errors-no-write/elm.json create mode 100644 test/project-with-suppressed-errors-no-write/expected-NoUnused.Variables.json create mode 100644 test/project-with-suppressed-errors-no-write/fixed-elm.json create mode 100644 test/project-with-suppressed-errors-no-write/review/elm.json create mode 100644 test/project-with-suppressed-errors-no-write/review/src/ReviewConfig.elm create mode 100644 test/project-with-suppressed-errors-no-write/review/suppressed/NoUnused.Variables.json create mode 100644 test/project-with-suppressed-errors-no-write/src/Main.elm create mode 100644 test/project-with-suppressed-errors-no-write/src/OtherFile.elm create mode 100644 test/project-with-suppressed-errors-no-write/with-errors-OtherFile.elm create mode 100644 test/snapshots/suppress/write-failure.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index 953a2ada..1143970b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## [Unreleased] +- Improve the error message when failing to update suppression files. + ## [2.11.0] - 2024-03-15 - Add an `offline` mode to prevent `elm-review` from making any HTTP requests. This is useful for CI environments that should not have access to the internet, where you only want to run `elm-review` without arguments. diff --git a/lib/suppressed-errors.js b/lib/suppressed-errors.js index 4cb615a7..16d8e31b 100644 --- a/lib/suppressed-errors.js +++ b/lib/suppressed-errors.js @@ -26,6 +26,8 @@ module.exports = { // WRITE +const orange = chalk.keyword('orange'); + /** * @param {Options} options * @param {Array} suppressionFiles @@ -55,7 +57,22 @@ async function write(options, suppressionFiles) { const previousContents = await FS.readFile(filePath).catch(() => ''); if (previousContents !== newContents) { - return FS.writeFile(filePath, newContents); + return FS.writeFile(filePath, newContents).catch((error) => { + const relativeFolderPath = + path.relative(process.cwd(), options.suppressedErrorsFolder()) + + '/'; + const relativeFilePath = path.relative(process.cwd(), filePath); + throw new ErrorMessage.CustomError( + 'FAILED TO UPDATE SUPPRESSION FILE', + // prettier-ignore + `I tried updating the suppression file in the ${orange(relativeFolderPath)} folder, but failed to write to ${orange(relativeFilePath)}. + +Please check that ${chalk.greenBright('elm-review')} has write permissions to that file and folder. In case it helps, here's the error I encountered: + + ${error.toString()}`, + filePath + ); + }); } return null; diff --git a/test/project-with-suppressed-errors-no-write/elm.json b/test/project-with-suppressed-errors-no-write/elm.json new file mode 100644 index 00000000..dea3450d --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/elm.json @@ -0,0 +1,24 @@ +{ + "type": "application", + "source-directories": [ + "src" + ], + "elm-version": "0.19.1", + "dependencies": { + "direct": { + "elm/browser": "1.0.2", + "elm/core": "1.0.5", + "elm/html": "1.0.0" + }, + "indirect": { + "elm/json": "1.1.3", + "elm/time": "1.0.0", + "elm/url": "1.0.0", + "elm/virtual-dom": "1.0.2" + } + }, + "test-dependencies": { + "direct": {}, + "indirect": {} + } +} diff --git a/test/project-with-suppressed-errors-no-write/expected-NoUnused.Variables.json b/test/project-with-suppressed-errors-no-write/expected-NoUnused.Variables.json new file mode 100644 index 00000000..e6854242 --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/expected-NoUnused.Variables.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "automatically created by": "elm-review suppress", + "learn more": "elm-review suppress --help", + "suppressions": [ + { "count": 2, "filePath": "src/Main.elm" } + ] +} diff --git a/test/project-with-suppressed-errors-no-write/fixed-elm.json b/test/project-with-suppressed-errors-no-write/fixed-elm.json new file mode 100644 index 00000000..dea3450d --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/fixed-elm.json @@ -0,0 +1,24 @@ +{ + "type": "application", + "source-directories": [ + "src" + ], + "elm-version": "0.19.1", + "dependencies": { + "direct": { + "elm/browser": "1.0.2", + "elm/core": "1.0.5", + "elm/html": "1.0.0" + }, + "indirect": { + "elm/json": "1.1.3", + "elm/time": "1.0.0", + "elm/url": "1.0.0", + "elm/virtual-dom": "1.0.2" + } + }, + "test-dependencies": { + "direct": {}, + "indirect": {} + } +} diff --git a/test/project-with-suppressed-errors-no-write/review/elm.json b/test/project-with-suppressed-errors-no-write/review/elm.json new file mode 100644 index 00000000..50ac3a89 --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/review/elm.json @@ -0,0 +1,34 @@ +{ + "type": "application", + "source-directories": [ + "src" + ], + "elm-version": "0.19.1", + "dependencies": { + "direct": { + "elm/core": "1.0.5", + "elm/json": "1.1.3", + "elm/project-metadata-utils": "1.0.2", + "jfmengels/elm-review": "2.13.0", + "jfmengels/elm-review-unused": "1.1.29", + "stil4m/elm-syntax": "7.2.9" + }, + "indirect": { + "elm/bytes": "1.0.8", + "elm/html": "1.0.0", + "elm/parser": "1.1.0", + "elm/random": "1.0.0", + "elm/time": "1.0.0", + "elm/virtual-dom": "1.0.3", + "elm-community/list-extra": "8.7.0", + "elm-explorations/test": "2.1.1", + "miniBill/elm-unicode": "1.0.3", + "rtfeldman/elm-hex": "1.0.0", + "stil4m/structured-writer": "1.0.3" + } + }, + "test-dependencies": { + "direct": {}, + "indirect": {} + } +} diff --git a/test/project-with-suppressed-errors-no-write/review/src/ReviewConfig.elm b/test/project-with-suppressed-errors-no-write/review/src/ReviewConfig.elm new file mode 100644 index 00000000..48ab4d47 --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/review/src/ReviewConfig.elm @@ -0,0 +1,24 @@ +module ReviewConfig exposing (config) + +{-| Do not rename the ReviewConfig module or the config function, because +`elm-review` will look for these. + +To add packages that contain rules, add them to this review project using + + `elm install author/packagename` + +when inside the directory containing this file. + +-} + + +import NoUnused.Dependencies +import NoUnused.Variables +import Review.Rule exposing (Rule) + + +config : List Rule +config = + [ NoUnused.Dependencies.rule + , NoUnused.Variables.rule + ] diff --git a/test/project-with-suppressed-errors-no-write/review/suppressed/NoUnused.Variables.json b/test/project-with-suppressed-errors-no-write/review/suppressed/NoUnused.Variables.json new file mode 100644 index 00000000..05686050 --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/review/suppressed/NoUnused.Variables.json @@ -0,0 +1,9 @@ +{ + "version": 1, + "automatically created by": "elm-review suppress", + "learn more": "elm-review suppress --help", + "suppressions": [ + { "count": 2, "filePath": "src/Main.elm" }, + { "count": 1, "filePath": "src/OtherFile.elm" } + ] +} diff --git a/test/project-with-suppressed-errors-no-write/src/Main.elm b/test/project-with-suppressed-errors-no-write/src/Main.elm new file mode 100644 index 00000000..d447fada --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/src/Main.elm @@ -0,0 +1,58 @@ +module Main exposing (main) + +import Browser +import Folder.Used exposing (one) +import Html + exposing + ( Html + , button + , div + , -- h1 is unused + h1 + , -- span is unused + span + , text + ) +import Html.Events exposing (onClick) + + +type alias Model = + { count : Int } + + +initialModel : Model +initialModel = + { count = 0 } + + +type Msg + = Increment + | Decrement + + +update : Msg -> Model -> Model +update msg model = + case msg of + Increment -> + { model | count = model.count + one } + + Decrement -> + { model | count = model.count - 1 } + + +view : Model -> Html Msg +view model = + div [] + [ button [ onClick Increment ] [ text "+1" ] + , div [] [ text <| String.fromInt model.count ] + , button [ onClick Decrement ] [ text "-1" ] + ] + + +main : Program () Model Msg +main = + Browser.sandbox + { init = initialModel + , view = view + , update = update + } diff --git a/test/project-with-suppressed-errors-no-write/src/OtherFile.elm b/test/project-with-suppressed-errors-no-write/src/OtherFile.elm new file mode 100644 index 00000000..37cdca39 --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/src/OtherFile.elm @@ -0,0 +1,5 @@ +module OtherFile exposing (a) + + +a = + 1 diff --git a/test/project-with-suppressed-errors-no-write/with-errors-OtherFile.elm b/test/project-with-suppressed-errors-no-write/with-errors-OtherFile.elm new file mode 100644 index 00000000..ca692e1b --- /dev/null +++ b/test/project-with-suppressed-errors-no-write/with-errors-OtherFile.elm @@ -0,0 +1,13 @@ +module OtherFile exposing (a) + + +a = + 1 + + +b = + 2 + + +c = + 3 diff --git a/test/snapshots/suppress/write-failure.txt b/test/snapshots/suppress/write-failure.txt new file mode 100644 index 00000000..77e018bc --- /dev/null +++ b/test/snapshots/suppress/write-failure.txt @@ -0,0 +1,8 @@ +-- FAILED TO UPDATE SUPPRESSION FILE ------------------------------------------- + +I tried updating the suppression file in the review/suppressed/ folder, but failed to write to review/suppressed/NoUnused.Variables.json. + +Please check that elm-review has write permissions to that file and folder. In case it helps, here's the error I encountered: + + Error: EACCES: permission denied, open '/test/project-with-suppressed-errors-no-write/review/suppressed/NoUnused.Variables.json' + diff --git a/test/suppress.test.js b/test/suppress.test.js index 80bf0b61..ed96a869 100644 --- a/test/suppress.test.js +++ b/test/suppress.test.js @@ -54,8 +54,17 @@ test('Running with "suppress --check-after-tests" when there are uncommitted cha }); test('Running with unsupported version of suppression files should exit with failure', async () => { - const output = await TestCli.runAndExpectError('', { - project: 'project-with-unsupported-suppression-version' - }); - expect(output).toMatchFile(testName('unsupported-suppression-version')); + // In this setup, running `elm-review` should update a suppression file because + // an unused variables issue has been fixed. It should however fail because + // write permission has been removed from `review/suppressed/NoUnused.Variables.json` + const project = 'project-with-suppressed-errors-no-write'; + const filePath = path.resolve( + __dirname, + project, + 'review/suppressed/NoUnused.Variables.json' + ); + childProcess.execSync(`chmod -w ${filePath}`); + + const output = await TestCli.runAndExpectError('', {project}); + expect(output).toMatchFile(testName('write-failure')); });