Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39092

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

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: JavaScript
    • Labels:

      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
      }
      

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dobedobedoh 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
            dobedobedoh 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
            dobedobedoh Andrew Nicols added a comment -

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

            Show
            dobedobedoh Andrew Nicols added a comment - Linking again MDLSITE-2210 to raise the recommended shifter version.
            Hide
            stronk7 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
            stronk7 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
            dobedobedoh 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
            dobedobedoh 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
            dobedobedoh 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
            dobedobedoh 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
            dobedobedoh Andrew Nicols added a comment -

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

            Show
            dobedobedoh Andrew Nicols added a comment - Damyon, do you want to Peer Review this, or will you do so on integration?
            Hide
            stronk7 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
            stronk7 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
            dobedobedoh 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
            dobedobedoh 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
            stronk7 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
            stronk7 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 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 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 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 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
            salvetore Michael de Raadt added a comment -

            Test result: Success!

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

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! I was shown some lint errors, but no coverage errors were reported.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  14/May/13