Moodle
  1. Moodle
  2. MDL-39092

Add a .shifter.json config to prevent build of code coverage reports.

    Details

    • Rank:
      49230

      Description

      Reasons:
      1. Default code coverage reports require java to build (stung me grr..)
      2. We aren't using them
      3. Diffs are big and ugly

      Suggestion:

      Add a .shifter.json file to the root moodle dir containing:

      {
          "coverage": false
      }
      

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          I am hoping that for 2.6 we can start writing unit tests for our common JS modules (notification, tooltip, chooserdialogue) initially and build them up to cover most code (especially file picker, filemanager, dock, etc). At this point, coverage will become invaluable.

          I'd like to suggest instead that we upgrade to a newer version of Shifter. We're currently recommending 0.2.28, but as of a couple of weeks ago with the 0.3 series, code coverage is now handled by Istanbul which is a pure JS coverage tool so Java is no longer required.

          Istanbul also spits out minified coverage JS so it should be much less intrusive on the diffs too.

          Show
          Andrew Nicols added a comment - I am hoping that for 2.6 we can start writing unit tests for our common JS modules (notification, tooltip, chooserdialogue) initially and build them up to cover most code (especially file picker, filemanager, dock, etc). At this point, coverage will become invaluable. I'd like to suggest instead that we upgrade to a newer version of Shifter. We're currently recommending 0.2.28, but as of a couple of weeks ago with the 0.3 series, code coverage is now handled by Istanbul which is a pure JS coverage tool so Java is no longer required. Istanbul also spits out minified coverage JS so it should be much less intrusive on the diffs too.
          Hide
          Andrew Nicols added a comment -

          Linking again MDLSITE-2210 to raise the recommended shifter version.

          Show
          Andrew Nicols added a comment - Linking again MDLSITE-2210 to raise the recommended shifter version.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 for:

          • go up to Shifter 0.3.x
          • stop building coverage files, deleting current ones by default and ignoring them.
          • a dev/ci wants to execute unit-testing/coverage reports, they will be able to do it explicitly.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 for: go up to Shifter 0.3.x stop building coverage files, deleting current ones by default and ignoring them. a dev/ci wants to execute unit-testing/coverage reports, they will be able to do it explicitly. Ciao
          Hide
          Andrew Nicols added a comment -

          What do we think about removing all of the -covarage.js files from the upstream YUI library too then?

          Show
          Andrew Nicols added a comment - What do we think about removing all of the -covarage.js files from the upstream YUI library too then?
          Hide
          Andrew Nicols added a comment -

          I decided to answer my own question. The upstream YUI coverage files take up approximately 15MB of space and we don't use them. There's absolutely no reason to keep them as part of Moodle.

          I've also taken the liberty of adding the lint: config option as this ensures that Shifter uses the Moodle-specific jshint configuration rather than it's default yui-lint configuration (ours differs slightly).

          Show
          Andrew Nicols added a comment - I decided to answer my own question. The upstream YUI coverage files take up approximately 15MB of space and we don't use them. There's absolutely no reason to keep them as part of Moodle. I've also taken the liberty of adding the lint: config option as this ensures that Shifter uses the Moodle-specific jshint configuration rather than it's default yui-lint configuration (ours differs slightly).
          Hide
          Andrew Nicols added a comment -

          Damyon, do you want to Peer Review this, or will you do so on integration?

          Show
          Andrew Nicols added a comment - Damyon, do you want to Peer Review this, or will you do so on integration?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          If we decide to take rid of the coverage files also from the yuilib/x.y.z import... then surely we should annotate it somewhere, coz, or I'm wrong or, until now, we import it 100% without any modifications.

          Show
          Eloy Lafuente (stronk7) added a comment - If we decide to take rid of the coverage files also from the yuilib/x.y.z import... then surely we should annotate it somewhere, coz, or I'm wrong or, until now, we import it 100% without any modifications.
          Hide
          Andrew Nicols added a comment -

          Point taken. I've added a note to lib/yuilib/readme_moodle.txt to state that we remove the coverage files but make no other changes.

          Show
          Andrew Nicols added a comment - Point taken. I've added a note to lib/yuilib/readme_moodle.txt to state that we remove the coverage files but make no other changes.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I keep this in Daymon hands, looks ok for me.

          Once integrated, plz, change the jobs configuration in the CI servers to 0.3.1... aka MDLSITE-2210.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I keep this in Daymon hands, looks ok for me. Once integrated, plz, change the jobs configuration in the CI servers to 0.3.1... aka MDLSITE-2210 . Ciao
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew,

          Agree with all comments above - I think this is much better.

          I'll integrate this right away to prevent conflicts.

          Show
          Damyon Wiese added a comment - Thanks Andrew, Agree with all comments above - I think this is much better. I'll integrate this right away to prevent conflicts.
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew, this has been integrated now - I deleted a few more (recently added) coverage files and slightly tweaked the readme_moodle.txt in case someone in future does not know what a coverage js file is.

          Show
          Damyon Wiese added a comment - Thanks Andrew, this has been integrated now - I deleted a few more (recently added) coverage files and slightly tweaked the readme_moodle.txt in case someone in future does not know what a coverage js file is.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          I was shown some lint errors, but no coverage errors were reported.

          Show
          Michael de Raadt added a comment - Test result: Success! I was shown some lint errors, but no coverage errors were reported.
          Hide
          Dan Poltawski added a comment -

          Blooming Marvelous! It's time for a knees up - your changes are upstream!

          Thanks for making Moodle better!

          Toodle pip

          Show
          Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: